[SPARK-33087][SQL] DataFrameWriterV2 should delegate table resolution to the analyzer by cloud-fan · Pull Request #29970 · apache/spark

cloud-fan · 2020-10-07T17:22:49Z

What changes were proposed in this pull request?

This PR makes DataFrameWriterV2 to create query plans with UnresolvedRelation and leave the table resolution work to the analyzer.

Why are the changes needed?

Table resolution work should be done by the analyzer. After this PR, the behavior is more consistent between different APIs (DataFrameWriter, DataFrameWriterV2 and SQL). See the next section for behavior changes.

Does this PR introduce any user-facing change?

Yes.

writes to a temp view of v2 relation: previously it fails with table not found exception, now it works if the v2 relation is writable. This is consistent with DataFrameWriter and SQL INSERT.
writes to other temp views: previously it fails with table not found exception, now it fails with a more explicit error message, saying that writing to a temp view of non-v2-relation is not allowed.
writes to a view: previously it fails with table not writable error, now it fails with a more explicit error message, saying that writing to a view is not allowed.
writes to a v1 table: previously it fails with table not writable error, now it fails with a more explicit error message, saying that writing to a v1 table is not allowed. (We can allow it later, by falling back to v1 command)

How was this patch tested?

new tests

cloud-fan · 2020-10-07T17:24:02Z

cc @HeartSaVioR @rdblue @brkyvz

SparkQA · 2020-10-07T18:10:09Z

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

brkyvz · 2020-10-07T18:12:01Z

 writeOptions: Map[String, String],
- isByName: Boolean) extends V2WriteCommand
+ isByName: Boolean) extends V2WriteCommand {
+ override def withNewTable(t: NamedRelation): AppendData = copy(table = t)


Can we just make the table be a child too and not add a special case for it in the Analyzer?

IIRC there was a discussion about it before and we decided to not making table as a child for writing commands. The same applies to InsertIntoStatement.
I can't clearly recall what the reason was, probably because we need to strip SubqueryAlias for table in the writing commands.

one related discussion: https://github.com/apache/spark/pull/21305/files#r206746383

Now I recalled. Scan nodes under writing command are special, as they are not for scan, and shouldn't apply things like filter pushdown.

SparkQA · 2020-10-07T18:28:40Z

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

SparkQA · 2020-10-07T21:57:01Z

Test build #129524 has finished for PR 29970 at commit b64f4bd.

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

HeartSaVioR

The content in tests in DataFrameWriterV2Suite are getting similar - it'd be nice if we can deduplicate across tests, but that is optional.

HeartSaVioR · 2020-10-24T03:24:06Z

retest this, please

SparkQA · 2020-10-24T03:37:34Z

Test build #130226 has finished for PR 29970 at commit b64f4bd.

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

HeartSaVioR · 2020-10-24T03:41:54Z

Looks like this needs rebasing with recent master. @cloud-fan

SparkQA · 2020-10-26T13:57:14Z

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

SparkQA · 2020-10-26T14:26:15Z

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

SparkQA · 2020-10-26T17:11:33Z

Test build #130286 has finished for PR 29970 at commit ee3587a.

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

HeartSaVioR · 2020-10-26T22:35:16Z

retest this, please

SparkQA · 2020-10-26T23:30:24Z

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

SparkQA · 2020-10-26T23:54:38Z

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

SparkQA · 2020-10-27T03:02:09Z

Test build #130300 has finished for PR 29970 at commit ee3587a.

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

HeartSaVioR

Looks OK to me, but probably I'm not quite qualified to make a decision.

cc. @brkyvz @rdblue Appreciate if you have time to revisit the PR. Thanks in advance.

cloud-fan · 2020-11-06T15:48:03Z

cc @dongjoon-hyun as well.

SparkQA · 2020-11-06T16:31:19Z

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

SparkQA · 2020-11-06T16:53:56Z

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

SparkQA · 2020-11-06T20:21:04Z

Test build #130730 has finished for PR 29970 at commit be50b21.

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

rdblue

+1

Looks good, thanks for fixing this. Please get tests passing before merging, of course.

HeartSaVioR · 2020-11-09T03:35:25Z

retest this, please

SparkQA · 2020-11-09T04:29:56Z

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

SparkQA · 2020-11-09T04:50:13Z

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

brkyvz

LGTM

SparkQA · 2020-11-09T08:05:02Z

Test build #130767 has finished for PR 29970 at commit be50b21.

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

cloud-fan · 2020-11-09T08:08:11Z

GA passed, merging to master, thanks for the reviews!

cloud-fan mentioned this pull request Oct 7, 2020
[SPARK-32960][SQL] Provide better exception on temporary view against DataFrameWriterV2 #29830
Closed

brkyvz reviewed Oct 7, 2020
View reviewed changes

HeartSaVioR reviewed Oct 8, 2020
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

dongjoon-hyun added the SQL label Oct 15, 2020

cloud-fan force-pushed the refactor branch from b64f4bd to ee3587aCompare October 26, 2020 12:33

HeartSaVioR reviewed Oct 27, 2020
View reviewed changes

DataFrameWriterV2 should delegate table resolution to the analyzer
be50b21

cloud-fan force-pushed the refactor branch from ee3587a to be50b21Compare November 6, 2020 15:45

rdblue approved these changes Nov 7, 2020
View reviewed changes

brkyvz approved these changes Nov 9, 2020
View reviewed changes

cloud-fan closed this in 98730b7Nov 9, 2020

Uh oh!

Conversation

cloud-fan commented Oct 7, 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?

Uh oh!

cloud-fan commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

brkyvzOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fanOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fanOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fanOct 26, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

SparkQA commented Oct 7, 2020

Uh oh!

HeartSaVioR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HeartSaVioR commented Oct 24, 2020

Uh oh!

SparkQA commented Oct 24, 2020

Uh oh!

HeartSaVioR commented Oct 24, 2020

Uh oh!

SparkQA commented Oct 26, 2020

Uh oh!

SparkQA commented Oct 26, 2020

Uh oh!

SparkQA commented Oct 26, 2020

Uh oh!

HeartSaVioR commented Oct 26, 2020

Uh oh!

SparkQA commented Oct 26, 2020

Uh oh!

SparkQA commented Oct 26, 2020

Uh oh!

SparkQA commented Oct 27, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 6, 2020

Uh oh!

SparkQA commented Nov 6, 2020

Uh oh!

SparkQA commented Nov 6, 2020

Uh oh!

SparkQA commented Nov 6, 2020

Uh oh!

rdblue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Nov 9, 2020

Uh oh!

SparkQA commented Nov 9, 2020

Uh oh!

SparkQA commented Nov 9, 2020

Uh oh!

brkyvz left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fanOct 26, 2020•
edited
Loading

HeartSaVioR left a comment •
edited
Loading

rdblue left a comment •
edited
Loading