[SPARK-25425][SQL] Extra options should override session options in DataSource V2 #22413

MaxGekk · 2018-09-13T15:23:00Z

What changes were proposed in this pull request?

In the PR, I propose overriding session options by extra options in DataSource V2. Extra options are more specific and set via .option(), and should overwrite more generic session options. Entries from seconds map overwrites entries with the same key from the first map, for example:

scala> Map("option" -> false) ++ Map("option" -> true)
res0: scala.collection.immutable.Map[String,Boolean] = Map(option -> true)

How was this patch tested?

Added a test for checking which option is propagated to a data source in load().

MaxGekk · 2018-09-13T15:23:21Z

@cloud-fan Please, take a look.

dongjoon-hyun · 2018-09-13T18:40:48Z

@MaxGekk . This looks worth for JIRA issue. Could you file a JIRA issue for this?

dongjoon-hyun · 2018-09-13T18:44:53Z

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

@@ -202,7 +202,7 @@ class DataFrameReader private[sql](sparkSession: SparkSession) extends Logging {
          DataSourceOptions.PATHS_KEY -> objectMapper.writeValueAsString(paths.toArray)
        }
        Dataset.ofRows(sparkSession, DataSourceV2Relation.create(
-          ds, extraOptions.toMap ++ sessionOptions + pathsOption,
+          ds, sessionOptions ++ extraOptions.toMap + pathsOption,


Also, cc @rdblue since this is introduced at aadf953#diff-f70bda59304588cc3abfa3a9840653f4R197 .

@dongjoon-hyun The commit didn't change semantic actually. Before it was:

(extraOptions ++ DataSourceV2Utils.extractSessionConfigs( ds = ds.asInstanceOf[DataSourceV2], conf = sparkSession.sessionState.conf))

Oh. It has more history. Thanks, @MaxGekk . Could you trace down when it started? We need to mark the affected version correctly in order to know the backport candidates.

Also, please add a test case for this. If this has a long history than SPARK-23203 (fixed at 2.4.0), we need to verify this during backporting.

The changes were added in #19861

Thank you. Then, it was 2.3.0.

SparkQA · 2018-09-13T19:29:29Z

Test build #96043 has finished for PR 22413 at commit 13729b5.

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

dongjoon-hyun · 2018-09-13T21:03:31Z

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


-          val relation = DataSourceV2Relation.create(source, options.toMap)
+          val relation = DataSourceV2Relation.create(source, options)


Both read/write-side tests.

+1 for tests

I wrote a test for read path since I was able to grab options propagated to DataSource but I have no idea so far for write path, only mocking probably. Does it make sense to do that?

For write path, I think we can use the traditional way instead of introducing mocking. Let me try.

SparkQA · 2018-09-13T22:32:36Z

Test build #96051 has finished for PR 22413 at commit a443054.

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

maropu · 2018-09-14T02:16:14Z

retest this please

SparkQA · 2018-09-14T06:04:38Z

Test build #96057 has finished for PR 22413 at commit a443054.

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

HyukjinKwon

Looks good but let me leave it to @cloud-fan

cloud-fan · 2018-09-14T15:46:24Z

good catch! I believe this was a mistake. LGTM

SparkQA · 2018-09-14T16:22:24Z

Test build #96078 has finished for PR 22413 at commit 48d4cef.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DataSourceWithSessionConfV2 extends SimpleDataSourceV2 with SessionConfigSupport

dongjoon-hyun · 2018-09-15T07:33:32Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2Suite.scala

@@ -385,6 +400,9 @@ class SimpleDataSourceV2 extends DataSourceV2 with BatchReadSupportProvider {
  }
 }

+class DataSourceWithSessionConfV2 extends SimpleDataSourceV2 with SessionConfigSupport {


DataSourceWithSessionConfV2 -> DataSourceV2WithSessionConfig?

dongjoon-hyun · 2018-09-15T07:41:38Z

@MaxGekk . Could you specify DSv2 more clearly in the PR title and description because it happens DSv2 code path only?

SparkQA · 2018-09-15T12:49:21Z

Test build #96091 has finished for PR 22413 at commit 83789c6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DataSourceV2WithSessionConfigs extends SimpleDataSourceV2 with SessionConfigSupport

dongjoon-hyun · 2018-09-15T19:08:50Z

sql/core/src/test/scala/org/apache/spark/sql/sources/v2/DataSourceV2Suite.scala

@@ -385,6 +400,9 @@ class SimpleDataSourceV2 extends DataSourceV2 with BatchReadSupportProvider {
  }
 }

+class DataSourceV2WithSessionConfigs extends SimpleDataSourceV2 with SessionConfigSupport {
+  def keyPrefix(): String = "test"
+}


Ur, it seems that we can use the existing one.

dongjoon-hyun · 2018-09-15T19:52:11Z

@MaxGekk . I made a write-path test case PR to you. Could you review that, MaxGekk#8 ?

@dongjoon-hyun

Add a test case for write path. Thank you very much, @dongjoon-hyun

dongjoon-hyun

+1, LGTM pending Jenkins tests.

SparkQA · 2018-09-15T23:15:57Z

Test build #96096 has finished for PR 22413 at commit eba46d9.

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

SparkQA · 2018-09-15T23:48:32Z

Test build #96098 has finished for PR 22413 at commit 325b9c4.

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

dongjoon-hyun · 2018-09-16T00:22:38Z

Thank you, @MaxGekk . Merged to master.

There is some conflict. Please make backporting PRs to 2.4 and 2.3.

rdblue · 2018-09-19T16:22:52Z

+1, thanks for fixing this, @dongjoon-hyun!

dongjoon-hyun · 2018-09-19T18:31:02Z

Oh, it was @MaxGekk . I helped some test cases. Anyway, thank you for your +1, @rdblue .
BTW, @MaxGekk . Could you send a backport PR to branch-2.4?

MaxGekk · 2018-09-19T18:50:49Z

There is some conflict. Please make backporting PRs to 2.4 and 2.3.
BTW, @MaxGekk . Could you send a backport PR to branch-2.4?

Sure, I will do that.

dongjoon-hyun · 2018-09-19T19:06:15Z

Thank you so much, @MaxGekk . It would be great if we can have that in Spark 2.4.0 RC2.

rdblue · 2018-09-19T19:07:04Z

Thanks @MaxGekk, sorry for the original omission!

…ataSource V2 In the PR, I propose overriding session options by extra options in DataSource V2. Extra options are more specific and set via `.option()`, and should overwrite more generic session options. Entries from seconds map overwrites entries with the same key from the first map, for example: ```Scala scala> Map("option" -> false) ++ Map("option" -> true) res0: scala.collection.immutable.Map[String,Boolean] = Map(option -> true) ``` Added a test for checking which option is propagated to a data source in `load()`. Closes apache#22413 from MaxGekk/session-options. Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Co-authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

MaxGekk · 2018-09-19T19:48:31Z

@dongjoon-hyun Here is the PR for 2.4: #22474 . I will check does it have conflicts in 2.3 and if so, I will backport it to 2.3

MaxGekk · 2018-09-19T20:52:18Z

@dongjoon-hyun It cannot be merged to 2.3 easily because DataSourceV2Relation doesn't have the options field, and the test for read is not compilable. I will try to fix it tomorrow.

dongjoon-hyun · 2018-09-19T21:01:04Z

I see. Thank you, @MaxGekk !

…ataSource V2 In the PR, I propose overriding session options by extra options in DataSource V2. Extra options are more specific and set via `.option()`, and should overwrite more generic session options. Entries from seconds map overwrites entries with the same key from the first map, for example: ```Scala scala> Map("option" -> false) ++ Map("option" -> true) res0: scala.collection.immutable.Map[String,Boolean] = Map(option -> true) ``` Added a test for checking which option is propagated to a data source in `load()`. Closes apache#22413 from MaxGekk/session-options. Lead-authored-by: Maxim Gekk <maxim.gekk@databricks.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Co-authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

Session options shouldn't override extra options

13729b5

dongjoon-hyun reviewed Sep 13, 2018

View reviewed changes

MaxGekk changed the title ~~Session options shouldn't override extra options~~ [SPARK-25425][SQL] Extra options overwrite session options Sep 13, 2018

MaxGekk added 2 commits September 13, 2018 22:24

Extra options should overwrite session options in save

14498fb

Remove a line

a443054

dongjoon-hyun reviewed Sep 13, 2018

View reviewed changes

HyukjinKwon approved these changes Sep 14, 2018

View reviewed changes

Test for load()

48d4cef

dongjoon-hyun reviewed Sep 15, 2018

View reviewed changes

MaxGekk changed the title ~~[SPARK-25425][SQL] Extra options overwrite session options~~ [SPARK-25425][SQL] Extra options override session options in DataSource V2 Sep 15, 2018

Renaming DataSourceWithSessionConfV2 to DataSourceV2WithSessionConfigs

83789c6

dongjoon-hyun reviewed Sep 15, 2018

View reviewed changes

MaxGekk and others added 2 commits September 15, 2018 21:26

Code de-duplication in tests

eba46d9

Add a test case for write path.

96990c7

dongjoon-hyun and others added 2 commits September 15, 2018 12:54

rename

d35d01b

Merge pull request #8 from dongjoon-hyun/SPARK-25425

325b9c4

Add a test case for write path. Thank you very much, @dongjoon-hyun

MaxGekk changed the title ~~[SPARK-25425][SQL] Extra options override session options in DataSource V2~~ [SPARK-25425][SQL] Extra options should override session options in DataSource V2 Sep 15, 2018

dongjoon-hyun approved these changes Sep 15, 2018

View reviewed changes

asfgit closed this in e06da95 Sep 16, 2018

MaxGekk mentioned this pull request Sep 20, 2018

[SPARK-25425][SQL][BACKPORT-2.3] Extra options should override session options in DataSource V2 #22489

Closed

MaxGekk deleted the session-options branch August 17, 2019 13:33


		val relation = DataSourceV2Relation.create(source, options.toMap)
		val relation = DataSourceV2Relation.create(source, options)

[SPARK-25425][SQL] Extra options should override session options in DataSource V2 #22413

[SPARK-25425][SQL] Extra options should override session options in DataSource V2 #22413

Conversation

MaxGekk commented Sep 13, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

MaxGekk commented Sep 13, 2018

dongjoon-hyun commented Sep 13, 2018

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

SparkQA commented Sep 13, 2018

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 Sep 13, 2018

maropu commented Sep 14, 2018

SparkQA commented Sep 14, 2018

HyukjinKwon left a comment

Choose a reason for hiding this comment

cloud-fan commented Sep 14, 2018

SparkQA commented Sep 14, 2018

Choose a reason for hiding this comment

dongjoon-hyun commented Sep 15, 2018

SparkQA commented Sep 15, 2018

Choose a reason for hiding this comment

dongjoon-hyun commented Sep 15, 2018

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Sep 15, 2018

SparkQA commented Sep 15, 2018

dongjoon-hyun commented Sep 16, 2018 • edited Loading

rdblue commented Sep 19, 2018

dongjoon-hyun commented Sep 19, 2018 • edited Loading

MaxGekk commented Sep 19, 2018

dongjoon-hyun commented Sep 19, 2018 • edited Loading

rdblue commented Sep 19, 2018

MaxGekk commented Sep 19, 2018

MaxGekk commented Sep 19, 2018

dongjoon-hyun commented Sep 19, 2018

MaxGekk commented Sep 13, 2018 •

edited

Loading

dongjoon-hyun commented Sep 16, 2018 •

edited

Loading

dongjoon-hyun commented Sep 19, 2018 •

edited

Loading

dongjoon-hyun commented Sep 19, 2018 •

edited

Loading