-
Notifications
You must be signed in to change notification settings - Fork 29.2k
[SPARK-30282][SQL] Migrate SHOW TBLPROPERTIES to new framework #26921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
5d8422d
db311fd
fc2c5a3
ae524c3
467145f
7b5816a
6de67d2
517793f
a845488
625596b
72b3307
4fe3e69
fb47ea9
1ff1dcd
23b743c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -201,22 +201,21 @@ class ResolveSessionCatalog( | |
| case RenameTableStatement(SessionCatalogAndTable(_, oldName), newNameParts, isView) => | ||
| AlterTableRenameCommand(oldName.asTableIdentifier, newNameParts.asTableIdentifier, isView) | ||
|
|
||
| case DescribeTableStatement( | ||
| nameParts @ SessionCatalogAndTable(catalog, tbl), partitionSpec, isExtended) => | ||
| loadTable(catalog, tbl.asIdentifier).collect { | ||
| case v1Table: V1Table => | ||
| DescribeTableCommand(tbl.asTableIdentifier, partitionSpec, isExtended) | ||
| }.getOrElse { | ||
| // The v1 `DescribeTableCommand` can describe view as well. | ||
| if (isView(tbl)) { | ||
| DescribeTableCommand(tbl.asTableIdentifier, partitionSpec, isExtended) | ||
| } else { | ||
| if (partitionSpec.nonEmpty) { | ||
| throw new AnalysisException("DESCRIBE TABLE does not support partition for v2 tables.") | ||
| case d @ DescribeTable(SessionCatalogAndResolvedTable(resolved), partitionSpec, isExtended) => | ||
| resolved.table match { | ||
| case _: V1Table => | ||
| DescribeTableCommand(getTableIdentifier(resolved), partitionSpec, isExtended) | ||
| case _ => | ||
| // The v1 `DescribeTableCommand` can describe view as well. | ||
| if (isView(resolved.identifier.asMultipartIdentifier)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. before we reach here, we may already fail with table/view mismatch.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I checked with hive: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-DescribeTable/View/MaterializedView/Column The DESCRIBE command works for both table and view, in Spark we have a weird parser rule We can keep the parser rule, but we should rename
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same for presto: https://prestodb.io/docs/current/sql/describe.html
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've open #27187 to refine it. This is a feature we missed when designing the new framework, so I opened a separated PR to update the framework to support accepting both table and view like DESCRIBE command. |
||
| DescribeTableCommand(getTableIdentifier(resolved), partitionSpec, isExtended) | ||
| } else { | ||
| if (partitionSpec.nonEmpty) { | ||
| throw new AnalysisException( | ||
| "DESCRIBE TABLE does not support partition for v2 tables.") | ||
| } | ||
| d | ||
| } | ||
| val r = UnresolvedV2Relation(nameParts, catalog.asTableCatalog, tbl.asIdentifier) | ||
| DescribeTable(r, isExtended) | ||
| } | ||
| } | ||
|
|
||
| case DescribeColumnStatement( | ||
|
|
@@ -483,10 +482,8 @@ class ResolveSessionCatalog( | |
| replace, | ||
| viewType) | ||
|
|
||
| case ShowTablePropertiesStatement(SessionCatalogAndTable(_, tbl), propertyKey) => | ||
| ShowTablePropertiesCommand( | ||
| tbl.asTableIdentifier, | ||
| propertyKey) | ||
| case ShowTableProperties(SessionCatalogAndResolvedTable(resolved), propertyKey) => | ||
| ShowTablePropertiesCommand(getTableIdentifier(resolved), propertyKey) | ||
|
|
||
| case DescribeFunctionStatement(CatalogAndIdentifier(catalog, ident), extended) => | ||
| val functionIdent = | ||
|
|
@@ -586,6 +583,16 @@ class ResolveSessionCatalog( | |
| } | ||
| } | ||
|
|
||
| object SessionCatalogAndResolvedTable { | ||
| def unapply(resolved: ResolvedTable): Option[ResolvedTable] = { | ||
| if (isSessionCatalog(resolved.catalog)) { | ||
| Some(resolved) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| object SessionCatalogAndNamespace { | ||
| def unapply(resolved: ResolvedNamespace): Option[(SupportsNamespaces, Seq[String])] = | ||
| if (isSessionCatalog(resolved.catalog)) { | ||
|
|
@@ -595,6 +602,11 @@ class ResolveSessionCatalog( | |
| } | ||
| } | ||
|
|
||
| private def getTableIdentifier(resolved: ResolvedTable): TableIdentifier = { | ||
|
imback82 marked this conversation as resolved.
Outdated
|
||
| assert(resolved.identifier.namespace.length < 2) | ||
| TableIdentifier(resolved.identifier.name, resolved.identifier.namespace.headOption) | ||
| } | ||
|
|
||
| private def assertTopLevelColumn(colName: Seq[String], command: String): Unit = { | ||
| if (colName.length > 1) { | ||
| throw new AnalysisException(s"$command does not support nested column: ${colName.quoted}") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.connector | ||
|
|
||
| import org.scalatest.BeforeAndAfter | ||
|
|
||
| import org.apache.spark.sql.{AnalysisException, QueryTest} | ||
| import org.apache.spark.sql.test.SharedSparkSession | ||
|
|
||
| class TableResolutionSuite extends QueryTest with SharedSparkSession with BeforeAndAfter{ | ||
|
imback82 marked this conversation as resolved.
Outdated
|
||
|
|
||
| before { | ||
| spark.conf.set("spark.sql.catalog.testcat", classOf[InMemoryTableCatalog].getName) | ||
| } | ||
|
|
||
| after { | ||
| spark.sessionState.catalog.reset() | ||
| spark.sessionState.catalogManager.reset() | ||
| spark.sessionState.conf.clear() | ||
| } | ||
|
|
||
| test("V2 commands should look up temp view first") { | ||
| val tbl = "t" | ||
| val commands = Seq( | ||
| s"DESCRIBE $tbl", | ||
| s"SHOW TBLPROPERTIES $tbl" | ||
| // s"ALTER TABLE $tbl ADD COLUMN data string" | ||
| ) | ||
|
|
||
| withTempView(s"$tbl") { | ||
| withTable(s"testcat.ns.$tbl") { | ||
| sql(s"CREATE TEMPORARY VIEW $tbl AS SELECT 1 AS i") | ||
| sql(s"CREATE TABLE testcat.ns.$tbl USING csv AS SELECT 2 AS i") | ||
| sql("USE testcat.ns") | ||
|
|
||
| commands.foreach { command => | ||
| val ex = intercept[AnalysisException] { | ||
| sql(command) | ||
| } | ||
| assert(ex.getMessage.contains(s"$tbl is a temp view not table.")) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems simpler to write