[SPARK-36850][SQL][FOLLOWUP] Simplify exception code and fix wrong condition for CTAS and RTAS #34857

huaxingao · 2021-12-10T02:14:28Z

What changes were proposed in this pull request?

fixed a few problems:

addressed this comment
combined several xxxOnlySupportedWithV2TableError
in CTAS and RTAS, the if isSessionCatalog(catalog) should not be on the pattern, it should be if (isSessionCatalog(catalog) && !isV2Provider(provider)). Otherwise, c.partitioning ++ c.tableSpec.bucketSpec.map(_.asTransform) is not done for non SessionCatalog case.
I tried this c.partitioning ++ c.tableSpec.bucketSpec.map(_.asTransform) inside AstBuilder but it failed here so I kept this in ResolveSessionCatalog

Why are the changes needed?

code cleaning up and bug fixing

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests

SparkQA · 2021-12-10T03:01:56Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50532/

SparkQA · 2021-12-10T04:02:04Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50532/

SparkQA · 2021-12-10T05:44:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50535/

huaxingao · 2021-12-10T06:29:15Z

@cloud-fan could you please take a look when you have a chance?

SparkQA · 2021-12-10T06:42:51Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50535/

SparkQA · 2021-12-10T07:12:48Z

Test build #146057 has finished for PR 34857 at commit b349352.

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

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

SparkQA · 2021-12-10T09:49:44Z

Test build #146060 has finished for PR 34857 at commit 7795d04.

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

…/ResolveSessionCatalog.scala Co-authored-by: Wenchen Fan <[email protected]>

SparkQA · 2021-12-10T17:18:39Z

Test build #146073 has finished for PR 34857 at commit 4e1d2e0.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-12-10T17:59:01Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50548/

SparkQA · 2021-12-10T18:01:25Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50551/

sunchao · 2021-12-10T18:21:49Z

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

      RefreshFunctionCommand(funcIdentifier.database, funcIdentifier.funcName)
  }

+  private def constructTableV1Cmd(


nit: constructV1TableCmd?

sunchao · 2021-12-10T18:22:22Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala

-
-  def alterQualifiedColumnOnlySupportedWithV2TableError(): Throwable = {
-    new AnalysisException("ALTER COLUMN with qualified column is only supported with v2 tables.")
+  def operationOnlySupportedWithV2TableError(message: String): Throwable = {


nit: message -> command or operation?

Fixed. thanks

SparkQA · 2021-12-10T18:47:01Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50551/

SparkQA · 2021-12-10T18:58:49Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50548/

SparkQA · 2021-12-10T20:49:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50552/

SparkQA · 2021-12-10T21:49:43Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50552/

SparkQA · 2021-12-10T21:59:18Z

Test build #146074 has finished for PR 34857 at commit a1bae12.

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

SparkQA · 2021-12-11T00:56:00Z

Test build #146077 has finished for PR 34857 at commit 765b8a3.

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

viirya · 2021-12-11T08:07:59Z

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


-    case c @ ReplaceTableAsSelect(ResolvedDBObjectName(catalog, _), _, _, _, _, _)
-        if isSessionCatalog(catalog) =>
+    case c @ ReplaceTableAsSelect(ResolvedDBObjectName(catalog, _), _, _, _, _, _) =>


So seems we don't have test for this case? Otherwise previously wrongly placed isSessionCatalog(catalog) should be detected?

…atalog CTAS and RTAS

SparkQA · 2021-12-11T17:51:20Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50573/

SparkQA · 2021-12-11T18:45:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50573/

SparkQA · 2021-12-11T22:04:12Z

Test build #146099 has finished for PR 34857 at commit 39874c4.

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

sunchao

LGTM

viirya · 2021-12-12T20:24:30Z

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

+    }
+  }
+
+  test("SPARK-34857: ReplaceTableAsSelect partitions can be specified using " +


SPARK-34857?

SparkQA · 2021-12-12T21:56:01Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50584/

SparkQA · 2021-12-12T22:38:12Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50584/

SparkQA · 2021-12-13T01:54:53Z

Test build #146110 has finished for PR 34857 at commit 1cc6c33.

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

cloud-fan · 2021-12-13T04:34:26Z

thanks, merging to master!

huaxingao · 2021-12-13T05:12:26Z

Thank you all so much!

github-actions bot added SQL STRUCTURED STREAMING labels Dec 10, 2021

[SPARK-36850][SQL][FOLLOWUP] Code clean up

7795d04

huaxingao force-pushed the followup branch from b349352 to 7795d04 Compare December 10, 2021 04:28

cloud-fan reviewed Dec 10, 2021

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala Outdated Show resolved Hide resolved

srowen approved these changes Dec 10, 2021

View reviewed changes

huaxingao and others added 2 commits December 10, 2021 09:11

Update sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis…

4e1d2e0

…/ResolveSessionCatalog.scala Co-authored-by: Wenchen Fan <[email protected]>

ConstructTableV1Cmd -> constructTableV1Cmd

a1bae12

sunchao reviewed Dec 10, 2021

View reviewed changes

address comments

765b8a3

viirya reviewed Dec 11, 2021

View reviewed changes

add tests to make sure partitions are set correctly for non-session c…

39874c4

…atalog CTAS and RTAS

srowen approved these changes Dec 12, 2021

View reviewed changes

sunchao approved these changes Dec 12, 2021

View reviewed changes

viirya reviewed Dec 12, 2021

View reviewed changes

fix jira number in newly added tests

1cc6c33

viirya approved these changes Dec 12, 2021

View reviewed changes

viirya changed the title ~~[SPARK-36850][SQL][FOLLOWUP] Code clean up~~ [SPARK-36850][SQL][FOLLOWUP] Simplify exception code and fix wrong condition for CTAS and RTAS Dec 12, 2021

cloud-fan approved these changes Dec 13, 2021

View reviewed changes

cloud-fan closed this in 1cc1459 Dec 13, 2021

huaxingao deleted the followup branch December 13, 2021 05:12

[SPARK-36850][SQL][FOLLOWUP] Simplify exception code and fix wrong condition for CTAS and RTAS #34857

[SPARK-36850][SQL][FOLLOWUP] Simplify exception code and fix wrong condition for CTAS and RTAS #34857

Uh oh!

Conversation

huaxingao commented Dec 10, 2021

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?

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

huaxingao commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

sunchao Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

huaxingao Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

sunchao Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

huaxingao Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 10, 2021

Uh oh!

SparkQA commented Dec 11, 2021

Uh oh!

viirya Dec 11, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 11, 2021

Uh oh!

SparkQA commented Dec 11, 2021

Uh oh!

SparkQA commented Dec 11, 2021

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

viirya Dec 12, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 12, 2021

Uh oh!

SparkQA commented Dec 12, 2021

Uh oh!

SparkQA commented Dec 13, 2021

Uh oh!

cloud-fan commented Dec 13, 2021

Uh oh!

huaxingao commented Dec 13, 2021

Uh oh!