[SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided #27642

imback82 · 2020-02-20T06:55:55Z

What changes were proposed in this pull request?

For the following:

CREATE TABLE t USING json AS SELECT 1 AS i
SELECT * FROM spark_catalog.t

spark_catalog.t is resolved to spark_catalog.default.t assuming the current namespace is default. However, this is not consistent with V2 behavior where the namespace must be specified if the catalog name is provided. This PR proposes to fix this inconsistency.

Why are the changes needed?

To be consistent with V2 table naming scheme in SQL commands.

Does this PR introduce any user-facing change?

Yes, now the user has to specify the namespace if the catalog name is provided. For example,

SELECT * FROM spark_catalog.t # Will throw AnalysisException with 'Session catalog cannot have an empty namespace: spark_catalog.t'
SELECT * FROM spark_catalog.default.t # OK

How was this patch tested?

Added new tests

imback82 · 2020-02-20T07:09:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/LookupCatalog.scala

        }
      }
+      if (CatalogV2Util.isSessionCatalog(catalog) && ident.namespace.isEmpty) {


@cloud-fan For a session catalog, I could make this assumption that the namespace is required, right? I looked at CatalogManager that uses v1SessionCatalog for setting current namespace if the current catalog is a session catalog; and v1SessionCatalog requires the namespace (database) to already exist.

shall we do this check inside SessionCatalogAndIdentifier?

or inside ResolveSessionCatalog

CatalogAndIdentifier should just focus on extracting catalog and identifier.

The reason I put here is that CatalogAndIdentifier is also used here:

spark/sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriterV2.scala

Lines 53 to 57 in 7c4ad63

private val (catalog, identifier) = {

val CatalogAndIdentifier(catalog, identifier) = tableName

(catalog.asTableCatalog, identifier)

}

, which means I have to put the same check everywhere CatalogAndIdentifier is used?

I think V2SessionCatalog should stop filling the default database. It should assume the input identifier is the final identifier like other catalogs, and fail if identifier doesn't have database part. Then we don't need to do the check here.

Good point, let me explore this route. Thanks!

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala

SparkQA · 2020-02-20T08:05:02Z

Test build #118701 has finished for PR 27642 at commit 018e6f6.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-02-21T04:28:17Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -396,7 +397,7 @@ class ResolveSessionCatalog(
      }

    case AnalyzeColumnStatement(tbl, columnNames, allColumns) =>
-      val v1TableName = parseV1Table(tbl, "ANALYZE TABLE")
+      val v1TableName = parseTempViewOrV1Table(tbl, "ANALYZE TABLE")


AnalyzeColumnCommand actually supports temp view.

Just want to check if I understand this correctly. This reason why we need to make this change is that parseV1Table would accidentally add currentName as the namespace for a temp view which is incorrect because temp view shouldn't have a namespace?

yes, and this command supports temp views.

imback82 · 2020-02-21T04:28:33Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -415,6 +416,10 @@ class ResolveSessionCatalog(
        partition)

    case ShowCreateTableStatement(tbl, asSerde) if !asSerde =>
+      if (isTempView(tbl)) {


This check is moved from ShowCreateTableCommand

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

imback82 · 2020-02-21T04:41:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

@@ -1085,47 +1085,42 @@ case class ShowCreateTableCommand(table: TableIdentifier)

  override def run(sparkSession: SparkSession): Seq[Row] = {
    val catalog = sparkSession.sessionState.catalog
-    if (catalog.isTemporaryTable(table)) {


The change is only removing this if block, but diff is bad.

shall we keep the if? it doesn't hurt.

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

cloud-fan · 2020-02-21T05:17:14Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -257,9 +257,10 @@ class ResolveSessionCatalog(
        case v1Table: V1Table =>
          DescribeColumnCommand(tbl.asTableIdentifier, colNameParts, isExtended)
      }.getOrElse {
-        if (isTempView(tbl)) {


can we follow how we deal with UncacheTableStatement? Basically just call parseTempViewOrV1Table

cloud-fan · 2020-02-21T05:20:10Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -659,14 +669,7 @@ class ResolveSessionCatalog(
  object SessionCatalogAndTable {
    def unapply(nameParts: Seq[String]): Option[(CatalogPlugin, Seq[String])] = nameParts match {
      case SessionCatalogAndIdentifier(catalog, ident) =>
-        if (nameParts.length == 1) {


so we are going to remove the hack in TempViewOrV1Table in a followup PR?

OK. I will do it as a follow up. (I couldn't just remove the check because of SPARK-30799: temp view name can't contain catalog name, but I will think about it as a follow up).

SparkQA · 2020-02-21T06:02:18Z

Test build #118751 has finished for PR 27642 at commit fea7909.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-21T08:00:42Z

Test build #118747 has finished for PR 27642 at commit 29c88f1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-21T08:05:02Z

Test build #118752 has finished for PR 27642 at commit 95d2297.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yuchenhuo

I'm not so familiar with this, but in general looks great! Left some questions. Thanks for doing this!

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

yuchenhuo · 2020-02-22T00:42:33Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -396,7 +397,7 @@ class ResolveSessionCatalog(
      }

    case AnalyzeColumnStatement(tbl, columnNames, allColumns) =>
-      val v1TableName = parseV1Table(tbl, "ANALYZE TABLE")
+      val v1TableName = parseTempViewOrV1Table(tbl, "ANALYZE TABLE")


Just want to check if I understand this correctly. This reason why we need to make this change is that parseV1Table would accidentally add currentName as the namespace for a temp view which is incorrect because temp view shouldn't have a namespace?

imback82 · 2020-02-23T03:42:16Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -396,7 +397,7 @@ class ResolveSessionCatalog(
      }

    case AnalyzeColumnStatement(tbl, columnNames, allColumns) =>
-      val v1TableName = parseV1Table(tbl, "ANALYZE TABLE")
+      val v1TableName = parseTempViewOrV1Table(tbl, "ANALYZE TABLE")


yes, and this command supports temp views.

imback82 · 2020-02-23T03:43:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

@@ -1085,47 +1085,42 @@ case class ShowCreateTableCommand(table: TableIdentifier)

  override def run(sparkSession: SparkSession): Seq[Row] = {
    val catalog = sparkSession.sessionState.catalog
-    if (catalog.isTemporaryTable(table)) {


imback82 · 2020-02-23T03:44:19Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -257,9 +257,10 @@ class ResolveSessionCatalog(
        case v1Table: V1Table =>
          DescribeColumnCommand(tbl.asTableIdentifier, colNameParts, isExtended)
      }.getOrElse {
-        if (isTempView(tbl)) {


sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

imback82 · 2020-02-23T04:15:05Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -659,14 +669,7 @@ class ResolveSessionCatalog(
  object SessionCatalogAndTable {
    def unapply(nameParts: Seq[String]): Option[(CatalogPlugin, Seq[String])] = nameParts match {
      case SessionCatalogAndIdentifier(catalog, ident) =>
-        if (nameParts.length == 1) {


OK. I will do it as a follow up. (I couldn't just remove the check because of SPARK-30799: temp view name can't contain catalog name, but I will think about it as a follow up).

imback82 · 2020-02-23T04:24:10Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/CachedTableSuite.scala

    // make sure table doesn't exist
    var e = intercept[AnalysisException](spark.table("nonexistentTable")).getMessage
-    assert(e.contains(expectedErrorMsg))
+    assert(e.contains(s"$expectedErrorMsg nonexistentTable"))


spark.table just creates UnresolvedRelation(multipartIdentifier), so it doesn't add current namespace.

SparkQA · 2020-02-23T05:18:59Z

Test build #118825 has finished for PR 27642 at commit 9a4671c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-23T05:26:23Z

Test build #118826 has finished for PR 27642 at commit 4e80fd2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-23T05:30:43Z

Test build #118827 has finished for PR 27642 at commit 1cf8907.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-23T17:23:47Z

Test build #118835 has finished for PR 27642 at commit ef83ba1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-02-23T22:17:55Z

Test build #118836 has finished for PR 27642 at commit bcd9a65.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-24T04:57:42Z

sql/core/src/test/resources/sql-tests/results/show-create-table.sql.out

@@ -44,7 +44,7 @@ SHOW CREATE TABLE tbl
 -- !query schema
 struct<createtab_stmt:string>
 -- !query output
-CREATE TABLE `tbl` (
+CREATE TABLE `default`.`tbl` (


not related to this PR but a future improvement to v2 commands: since we resolve the catalog and tables during the analysis phase, it would be better to display the fully qualified table name(include catalog name) in EXPLAIN, to let users know which table exactly was picked by the command.

cloud-fan · 2020-02-24T05:07:51Z

sql/core/src/test/scala/org/apache/spark/sql/connector/TestV2SessionCatalogBase.scala

@@ -43,7 +44,7 @@ private[connector] trait TestV2SessionCatalogBase[T <: Table] extends Delegating

  protected def fullIdentifier(ident: Identifier): Identifier = {


We can remove this method completely.

Good catch! Removed.

cloud-fan · 2020-02-24T05:09:51Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSessionCatalogSuite.scala

+    val ident = if (v2Catalog.name == SESSION_CATALOG_NAME) {
+      Identifier.of(nameParts.init.toArray, nameParts.last)
+    } else {
+      Identifier.of(Array.empty, nameParts.last)


Can't we write Identifier.of(nameParts.init.toArray, nameParts.last) for the else branch as well?

You are right. Updated.

SparkQA · 2020-02-24T08:05:02Z

Test build #118848 has finished for PR 27642 at commit b43890a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-24T08:49:01Z

retest this please

SparkQA · 2020-02-24T13:30:12Z

Test build #118858 has finished for PR 27642 at commit b43890a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-24T13:58:31Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -415,6 +404,10 @@ class ResolveSessionCatalog(
        partition)

    case ShowCreateTableStatement(tbl, asSerde) if !asSerde =>
+      if (isTempView(tbl)) {
+        throw new AnalysisException(


actually, can we just call parseTempViewOrV1Table here?

e.g.

val name = parseTempViewOrV1Table... ShowCreateTableCommand(name.asTableIdentifier, ...)

OK.

That was my original implementation, but I did it this way because you will get SHOW CREATE TABLE is only supported with temp views or v1 tables then if you pass temp view, you get SHOW CREATE TABLE is not supported on a temporary view.

But I will just call parseTempVieworV1Table here. Thanks!

cloud-fan · 2020-02-24T14:03:29Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSessionCatalogSuite.scala


 class DataSourceV2SQLSessionCatalogSuite
  extends InsertIntoTests(supportsDynamicOverwrite = true, includeSQLOnlyTests = true)
  with AlterTableTests
  with SessionCatalogTest[InMemoryTable, InMemoryTableSessionCatalog] {

-  override protected val catalogAndNamespace = ""
+  override protected val catalogAndNamespace = "default."


thinking about this more. can we avoid changing it by updating the test cases that check error message?

I've already seen similar code, e.g. in InsertIntoTests

val tableName = if (catalogAndNamespace.isEmpty) s"default.$t1" else t1 assert(exc.getMessage.contains(s"Cannot write to '$tableName', too many data columns"))

Yea makes sense. Thanks for pointing this out.

SparkQA · 2020-02-24T21:32:33Z

Test build #118881 has finished for PR 27642 at commit eb0ebf1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yuchenhuo

LGTM+1! Thanks for doing this!

cloud-fan · 2020-02-25T05:04:43Z

thanks, merging to master/3.0!

… name is provided For the following: ``` CREATE TABLE t USING json AS SELECT 1 AS i SELECT * FROM spark_catalog.t ``` `spark_catalog.t` is resolved to `spark_catalog.default.t` assuming the current namespace is `default`. However, this is not consistent with V2 behavior where the namespace must be specified if the catalog name is provided. This PR proposes to fix this inconsistency. To be consistent with V2 table naming scheme in SQL commands. Yes, now the user has to specify the namespace if the catalog name is provided. For example, ``` SELECT * FROM spark_catalog.t # Will throw AnalysisException with 'Session catalog cannot have an empty namespace: spark_catalog.t' SELECT * FROM spark_catalog.default.t # OK ``` Added new tests Closes #27642 from imback82/disallow_spark_catalog_wihtout_db. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 0fd4fa7) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… tables that are not fully qualified ### What changes were proposed in this pull request? There are few V1 commands such as `REFRESH TABLE` that still allow `spark_catalog.t` because they run the commands with parsed table names without trying to load them in the catalog. This PR addresses this issue. The PR also addresses the issue brought up in #27642 (comment). ### Why are the changes needed? To fix a bug where for some V1 commands, `spark_catalog.t` is allowed. ### Does this PR introduce any user-facing change? Yes, a bug is fixed and `REFRESH TABLE spark_catalog.t` is not allowed. ### How was this patch tested? Added new test. Closes #27718 from imback82/fix_TempViewOrV1Table. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… tables that are not fully qualified ### What changes were proposed in this pull request? There are few V1 commands such as `REFRESH TABLE` that still allow `spark_catalog.t` because they run the commands with parsed table names without trying to load them in the catalog. This PR addresses this issue. The PR also addresses the issue brought up in #27642 (comment). ### Why are the changes needed? To fix a bug where for some V1 commands, `spark_catalog.t` is allowed. ### Does this PR introduce any user-facing change? Yes, a bug is fixed and `REFRESH TABLE spark_catalog.t` is not allowed. ### How was this patch tested? Added new test. Closes #27718 from imback82/fix_TempViewOrV1Table. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit b302781) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… name is provided ### What changes were proposed in this pull request? For the following: ``` CREATE TABLE t USING json AS SELECT 1 AS i SELECT * FROM spark_catalog.t ``` `spark_catalog.t` is resolved to `spark_catalog.default.t` assuming the current namespace is `default`. However, this is not consistent with V2 behavior where the namespace must be specified if the catalog name is provided. This PR proposes to fix this inconsistency. ### Why are the changes needed? To be consistent with V2 table naming scheme in SQL commands. ### Does this PR introduce any user-facing change? Yes, now the user has to specify the namespace if the catalog name is provided. For example, ``` SELECT * FROM spark_catalog.t # Will throw AnalysisException with 'Session catalog cannot have an empty namespace: spark_catalog.t' SELECT * FROM spark_catalog.default.t # OK ``` ### How was this patch tested? Added new tests Closes apache#27642 from imback82/disallow_spark_catalog_wihtout_db. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… tables that are not fully qualified ### What changes were proposed in this pull request? There are few V1 commands such as `REFRESH TABLE` that still allow `spark_catalog.t` because they run the commands with parsed table names without trying to load them in the catalog. This PR addresses this issue. The PR also addresses the issue brought up in apache#27642 (comment). ### Why are the changes needed? To fix a bug where for some V1 commands, `spark_catalog.t` is allowed. ### Does this PR introduce any user-facing change? Yes, a bug is fixed and `REFRESH TABLE spark_catalog.t` is not allowed. ### How was this patch tested? Added new test. Closes apache#27718 from imback82/fix_TempViewOrV1Table. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

initial commit

018e6f6

imback82 commented Feb 20, 2020

View reviewed changes

cloud-fan reviewed Feb 20, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Feb 20, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala Outdated Show resolved Hide resolved

address PR comments

29c88f1

imback82 changed the title ~~[SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided~~ [WIP][SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided Feb 21, 2020

test fixes

fea7909

imback82 changed the title ~~[WIP][SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided~~ [SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided Feb 21, 2020

remove dead code

95d2297

imback82 commented Feb 21, 2020

View reviewed changes

cloud-fan reviewed Feb 21, 2020

View reviewed changes

yuchenhuo reviewed Feb 22, 2020

View reviewed changes

imback82 added 3 commits February 22, 2020 19:29

fix tests, address comments

9a4671c

address PR comments

4e80fd2

address pr comments

1cf8907

imback82 commented Feb 23, 2020

View reviewed changes

imback82 added 2 commits February 23, 2020 08:02

Merge branch 'master' into disallow_spark_catalog_wihtout_db

cc17989

fix test

ef83ba1

fix test

bcd9a65

cloud-fan reviewed Feb 24, 2020

View reviewed changes

address PR comments

b43890a

cloud-fan approved these changes Feb 24, 2020

View reviewed changes

cloud-fan reviewed Feb 24, 2020

View reviewed changes

Address PR comments

eb0ebf1

yuchenhuo approved these changes Feb 25, 2020

View reviewed changes

cloud-fan closed this in 0fd4fa7 Feb 25, 2020

imback82 mentioned this pull request Feb 27, 2020

[SPARK-30885][SQL][FOLLOW-UP] Fix issues where some V1 commands allow tables that are not fully qualified. #27718

Closed


	private val (catalog, identifier) = {
	val CatalogAndIdentifier(catalog, identifier) = tableName
	(catalog.asTableCatalog, identifier)
	}

		@@ -43,7 +44,7 @@ private[connector] trait TestV2SessionCatalogBase[T <: Table] extends Delegating

		protected def fullIdentifier(ident: Identifier): Identifier = {

[SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided #27642

[SPARK-30885][SQL] V1 table name should be fully qualified if catalog name is provided #27642

Conversation

imback82 commented Feb 20, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Feb 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Feb 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 21, 2020

SparkQA commented Feb 21, 2020

SparkQA commented Feb 21, 2020

yuchenhuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imback82 Feb 23, 2020 • edited

Choose a reason for hiding this comment

SparkQA commented Feb 23, 2020

SparkQA commented Feb 23, 2020

SparkQA commented Feb 23, 2020

SparkQA commented Feb 23, 2020

SparkQA commented Feb 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 24, 2020

cloud-fan commented Feb 24, 2020

SparkQA commented Feb 24, 2020

cloud-fan Feb 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 24, 2020

yuchenhuo left a comment

Choose a reason for hiding this comment

cloud-fan commented Feb 25, 2020

cloud-fan Feb 20, 2020 •

edited

cloud-fan Feb 21, 2020 •

edited

imback82 Feb 23, 2020 •

edited

cloud-fan Feb 24, 2020 •

edited