[SPARK-51281][SQL] DataFrameWriterV2 should respect the path option #50040

cloud-fan · 2025-02-21T08:47:54Z

What changes were proposed in this pull request?

Unlike DataFrameWriter.saveAsTable where we explicitly get the "path" option and treat it as table location, DataFrameWriterV2 doesn't do it and treats the "path" option as a normal option which doesn't have any real impact.

This PR fixes it, and adds a legacy config to restore the old behavior.

Why are the changes needed?

bug fix

Does this PR introduce any user-facing change?

Yes, now DataFrameWriterV2 can correctly write data to the specified path for file source tables.

How was this patch tested?

new test

Was this patch authored or co-authored using generative AI tooling?

no

cloud-fan · 2025-02-21T08:48:29Z

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

+      properties = properties.toMap,
+      provider = provider,
+      optionExpression = OptionList(Seq.empty),
+      location = if (ignorePathOption) None else CaseInsensitiveMap(options.toMap).get("path"),


This is the actual change.

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

…Suite.scala

cloud-fan · 2025-02-21T08:50:33Z

@dongjoon-hyun shall we include it in 3.5.5? Also cc @aokolnychyi

dongjoon-hyun · 2025-02-21T16:06:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      .internal()
+      .doc("When set to true, DataFrameWriterV2 ignores the 'path' option and always write data " +
+        "to the default table location.")
+      .version("4.0.0")


+1 for 3.5.5. BTW, @cloud-fan , if we want to include this, we need to change this to 3.5.5.

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

dongjoon-hyun · 2025-02-24T04:14:15Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

+      withSQLConf(SQLConf.LEGACY_DF_WRITER_V2_IGNORE_PATH_OPTION.key -> ignorePath.toString) {
+        withTable("t1", "t2") {
+          spark.range(10).writeTo("t1").using("json").create()
+          checkAnswer(spark.table("t1"), spark.range(10).toDF())


nit. spark.range(10).toDF() seems to be repeated three times. Shall we define one to reuse during this unit test?

beliefer · 2025-02-24T05:38:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+    buildConf("spark.sql.legacy.dataFrameWriterV2IgnorePathOption")
+      .internal()
+      .doc("When set to true, DataFrameWriterV2 ignores the 'path' option and always write data " +
+        "to the default table location.")


Could we fix the bug? It means users might already use the 'path' option.

I got it. The bug exists about 3 years. So let the bug as a legacy behavior looks better.

cloud-fan · 2025-02-24T13:23:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      .internal()
+      .doc("When set to true, DataFrameWriterV2 ignores the 'path' option and always write data " +
+        "to the default table location.")
+      .version("3.5.5")


I'll change it to 3.5.6 after the 3.5.5 RC1 vote passes.

I'll change it to 3.5.6 after the 3.5.5 RC1 vote passes.

Ya, let's wait and see the vote result because it's still open.

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

szehon-ho

nice find

szehon-ho · 2025-02-25T06:38:38Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

      parameters = Map("methodName" -> "`writeTo`"))
  }
+
+  test("SPARK-51281: create/replace file source tables") {


question: are we testing replace here? Why not just the name of the jira?

manuzhang · 2025-02-25T08:30:00Z

BTW, do we have documentations on DataFrameWriterV2 API? I don't find any from searching on the Spark website.

cloud-fan · 2025-02-26T10:41:09Z

We only have the API doc: https://spark.apache.org/docs/latest/api/scala/org/apache/spark/sql/DataFrameWriterV2.html

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala

…Suite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

…onf.scala

cloud-fan · 2025-02-27T13:24:27Z

thanks for the review, merging to master/4.0/3.5!

### What changes were proposed in this pull request? Unlike `DataFrameWriter.saveAsTable` where we explicitly get the "path" option and treat it as table location, `DataFrameWriterV2` doesn't do it and treats the "path" option as a normal option which doesn't have any real impact. This PR fixes it, and adds a legacy config to restore the old behavior. ### Why are the changes needed? bug fix ### Does this PR introduce _any_ user-facing change? Yes, now `DataFrameWriterV2` can correctly write data to the specified path for file source tables. ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #50040 from cloud-fan/prop. Lead-authored-by: Wenchen Fan <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a3671e5) Signed-off-by: Wenchen Fan <[email protected]>

Unlike `DataFrameWriter.saveAsTable` where we explicitly get the "path" option and treat it as table location, `DataFrameWriterV2` doesn't do it and treats the "path" option as a normal option which doesn't have any real impact. This PR fixes it, and adds a legacy config to restore the old behavior. bug fix Yes, now `DataFrameWriterV2` can correctly write data to the specified path for file source tables. new test no Closes #50040 from cloud-fan/prop. Lead-authored-by: Wenchen Fan <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a3671e5) Signed-off-by: Wenchen Fan <[email protected]>

yaooqinn · 2025-03-05T06:30:06Z

Hi, https://github.com/apache/spark/actions/runs/13567662565/job/37924455859

The doc build CI for 3.5 seems broken after this PR merged

cloud-fan · 2025-03-05T09:02:38Z

hmm how can it be? This PR doesn't touch any doc...

dongjoon-hyun · 2025-03-06T02:04:25Z

@cloud-fan . It's a unidoc failure and it happens on all commit builder and PR builder (on branch-3.5) after this PR.

[info] Main Scala API documentation to /__w/spark/spark/target/scala-2.12/unidoc...
[info] Main Java API documentation to /__w/spark/spark/target/javaunidoc...
[error] /__w/spark/spark/common/utils/target/java/org/apache/spark/ErrorInfo.java:11:1:  error: illegal combination of modifiers: abstract and static
...
[warn] javadoc exited with exit code 1
...
[error] (Javaunidoc / doc) sbt.inc.Doc$JavadocGenerationFailed
[error] Total time: 404 s (06:44), completed Mar 5, 2025 6:07:14 PM

dongjoon-hyun · 2025-03-06T02:39:18Z

I verified that the reverting recovers unidoc.

$ git log --oneline -n1
c8c0f1feb0f (HEAD -> branch-3.5) Revert "[SPARK-51281][SQL] DataFrameWriterV2 should respect the path option"

$ ./build/sbt unidoc
...
[info] Main Scala API documentation successful.
[success] Total time: 242 s (04:02), completed Mar 5, 2025 6:12:13 PM

Let me revert this from branch-3.5 to recover branch-3.5 CI and unblock other PRs.

dongjoon-hyun · 2025-03-06T02:41:00Z

Now, this is reverted from branch-3.5 via c8c0f1f due to the CI failure.

Please make a backporting PR to branch-3.5 once more, @cloud-fan .

dongjoon-hyun · 2025-03-06T04:15:36Z

For the record, branch-3.5 is recovered.

Unlike `DataFrameWriter.saveAsTable` where we explicitly get the "path" option and treat it as table location, `DataFrameWriterV2` doesn't do it and treats the "path" option as a normal option which doesn't have any real impact. This PR fixes it, and adds a legacy config to restore the old behavior. bug fix Yes, now `DataFrameWriterV2` can correctly write data to the specified path for file source tables. new test no Closes apache#50040 from cloud-fan/prop. Lead-authored-by: Wenchen Fan <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a3671e5) Signed-off-by: Wenchen Fan <[email protected]>

backport #50040 to 3.5 ### What changes were proposed in this pull request? Unlike `DataFrameWriter.saveAsTable` where we explicitly get the "path" option and treat it as table location, `DataFrameWriterV2` doesn't do it and treats the "path" option as a normal option which doesn't have any real impact. This PR fixes it, and adds a legacy config to restore the old behavior. ### Why are the changes needed? bug fix ### Does this PR introduce _any_ user-facing change? Yes, now `DataFrameWriterV2` can correctly write data to the specified path for file source tables. ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #50179 from cloud-fan/backport. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ath option (apache#705) * [SPARK-51281][SQL] DataFrameWriterV2 should respect the path option Unlike `DataFrameWriter.saveAsTable` where we explicitly get the "path" option and treat it as table location, `DataFrameWriterV2` doesn't do it and treats the "path" option as a normal option which doesn't have any real impact. This PR fixes it, and adds a legacy config to restore the old behavior. bug fix Yes, now `DataFrameWriterV2` can correctly write data to the specified path for file source tables. new test no Closes apache#50040 from cloud-fan/prop. Lead-authored-by: Wenchen Fan <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit a3671e5) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? Unlike `DataFrameWriter.saveAsTable` where we explicitly get the "path" option and treat it as table location, `DataFrameWriterV2` doesn't do it and treats the "path" option as a normal option which doesn't have any real impact. This PR fixes it, and adds a legacy config to restore the old behavior. ### Why are the changes needed? bug fix ### Does this PR introduce _any_ user-facing change? Yes, now `DataFrameWriterV2` can correctly write data to the specified path for file source tables. ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#50040 from cloud-fan/prop. Lead-authored-by: Wenchen Fan <[email protected]> Co-authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit b23ae55) Signed-off-by: Wenchen Fan <[email protected]>

DataFrameWriterV2 should respect the path option

feff1e4

github-actions bot added the SQL label Feb 21, 2025

cloud-fan commented Feb 21, 2025

View reviewed changes

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

Update sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2…

215a606

…Suite.scala

dongjoon-hyun reviewed Feb 21, 2025

View reviewed changes

cloud-fan commented Feb 24, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

Apply suggestions from code review

88299af

dongjoon-hyun reviewed Feb 24, 2025

View reviewed changes

beliefer reviewed Feb 24, 2025

View reviewed changes

Update DataFrameWriterV2Suite.scala

8f12984

cloud-fan commented Feb 24, 2025

View reviewed changes

beliefer reviewed Feb 24, 2025

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2Suite.scala Show resolved Hide resolved

Update DataFrameWriterV2Suite.scala

d2cc6e5

szehon-ho approved these changes Feb 25, 2025

View reviewed changes

cloud-fan commented Feb 26, 2025

View reviewed changes

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

Update sql/core/src/test/scala/org/apache/spark/sql/DataFrameWriterV2…

f046b2e

…Suite.scala

cloud-fan commented Feb 27, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

Update sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLC…

8702883

…onf.scala

cloud-fan closed this in a3671e5 Feb 27, 2025

cloud-fan mentioned this pull request Mar 6, 2025

[SPARK-51281][SQL][3.5] DataFrameWriterV2 should respect the path option #50179

Closed

[SPARK-51281][SQL] DataFrameWriterV2 should respect the path option #50040

[SPARK-51281][SQL] DataFrameWriterV2 should respect the path option #50040

Uh oh!

Conversation

cloud-fan commented Feb 21, 2025

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?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

cloud-fan Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cloud-fan commented Feb 21, 2025

Uh oh!

dongjoon-hyun Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun Feb 24, 2025 • 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!

beliefer Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

beliefer Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szehon-ho left a comment

Choose a reason for hiding this comment

Uh oh!

szehon-ho Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

manuzhang commented Feb 25, 2025

Uh oh!

cloud-fan commented Feb 26, 2025

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Feb 27, 2025

Uh oh!

yaooqinn commented Mar 5, 2025

Uh oh!

cloud-fan commented Mar 5, 2025

Uh oh!

dongjoon-hyun commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Mar 6, 2025

Uh oh!

dongjoon-hyun commented Mar 6, 2025

Uh oh!

dongjoon-hyun commented Mar 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dongjoon-hyun Feb 24, 2025 •

edited

Loading

dongjoon-hyun commented Mar 6, 2025 •

edited

Loading