[SPARK-32592][SQL] Make DataFrameReader.table take the specified options #29535

huaxingao · 2020-08-25T00:06:20Z

What changes were proposed in this pull request?

pass specified options in DataFrameReader.table to JDBCTableCatalog.loadTable

Why are the changes needed?

Currently, DataFrameReader.table ignores the specified options. The options specified like the following are lost.

    val df = spark.read
      .option("partitionColumn", "id")
      .option("lowerBound", "0")
      .option("upperBound", "3")
      .option("numPartitions", "2")
      .table("h2.test.people")

We need to make DataFrameReader.table take the specified options.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually test for now. Will add a test after V2 JDBC read is implemented.

SparkQA · 2020-08-25T05:18:26Z

Test build #127860 has finished for PR 29535 at commit b54cc55.

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

huaxingao · 2020-08-25T05:24:45Z

cc @MaxGekk @cloud-fan @viirya

cloud-fan · 2020-08-25T05:55:59Z

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

@@ -822,6 +821,8 @@ class DataFrameReader private[sql](sparkSession: SparkSession) extends Logging {
   */
  def table(tableName: String): DataFrame = {
    assertNoSpecifiedSchema("table")
+    for ((k, v) <- this.extraOptions)
+      sparkSession.conf.set(k, v)


We need to clearly define the behavior here.

I think the options don't make sense if we read temp view, view, or v1 tables in the session catalog. For v1 table, they are created with CREATE TABLE ... USING ... OPTIONS ..., so the options are already there and it's confusing to overwrite them per scan.

We do need to keep the scan options when reading v2 tables, but I don't think session conf is the right channel. I think we should put the options in UnresolvedRelation and apply these options when we resolve UnresolvedRelation to v2 relations.

@cloud-fan Thanks for your comment.
I took a look at the code. I think I need to change the following:

I need to modify lots of files because of the above changes. Could you please check if this looks OK to you before I continue? Thank you very much!

We don't need to change SparkSession.table. We can create UnresolvedRelation in DataFrameReader.table directly.

This is options to scan the v2 table, not v2 table properties. We don't need to change TableCatalog.loadTable, but pass the scan options to Table.newScanBuilder. DataSourceV2Relation already takes the scan options, we just need to create DataSourceV2Relation with scan options in ResolveTables and ResolveRelations.

Currently, we already pass options from DataFrameReader to DataSourceV2Relation in load API. DataFrameReader.table misses this part.

SparkQA · 2020-08-26T23:57:23Z

Test build #127942 has finished for PR 29535 at commit 05b55cb.

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

SparkQA · 2020-08-27T04:54:56Z

Test build #127943 has finished for PR 29535 at commit 0bc7698.

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

huaxingao · 2020-08-27T05:04:49Z

@cloud-fan @viirya Could you please check one more time? Thanks!

cloud-fan · 2020-08-27T06:29:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

@@ -42,7 +42,9 @@ class UnresolvedException[TreeType <: TreeNode[_]](tree: TreeType, function: Str
 * @param multipartIdentifier table name
 */
 case class UnresolvedRelation(
-    multipartIdentifier: Seq[String]) extends LeafNode with NamedRelation {
+    multipartIdentifier: Seq[String],
+    options: CaseInsensitiveMap[String] = CaseInsensitiveMap[String](Map.empty))


can we use CaseInsensitiveStringMap since it's only for v2?

cloud-fan · 2020-08-27T06:31:06Z

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

-    sparkSession.table(tableName)
+    val multipartIdentifier =
+      sparkSession.sessionState.sqlParser.parseMultipartIdentifier(tableName)
+    Dataset.ofRows(sparkSession, UnresolvedRelation(multipartIdentifier, extraOptions))


we should create CaseInsensitiveStringMap here:

new CaseInsensitiveStringMap(extraOptions.toMap.asJava)

toMap is required to get the original map (keys are not lowercased)

viirya · 2020-08-27T05:30:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

@@ -42,7 +42,9 @@ class UnresolvedException[TreeType <: TreeNode[_]](tree: TreeType, function: Str
 * @param multipartIdentifier table name


Add options to param doc?

huaxingao · 2020-08-27T07:32:10Z

Fixed. Thank you! @cloud-fan @viirya

SparkQA · 2020-08-27T07:34:13Z

Test build #127948 has finished for PR 29535 at commit d56f92d.

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

cloud-fan · 2020-08-27T07:34:50Z

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

-    private def lookupV2Relation(identifier: Seq[String]): Option[DataSourceV2Relation] =
+    private def lookupV2Relation(
+        identifier: Seq[String],
+        options: CaseInsensitiveStringMap = CaseInsensitiveStringMap.empty())


do we really need the default value?

cloud-fan · 2020-08-27T07:35:03Z

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

-    private def lookupRelation(identifier: Seq[String]): Option[LogicalPlan] = {
+    private def lookupRelation(
+        identifier: Seq[String],
+        options: CaseInsensitiveStringMap = CaseInsensitiveStringMap.empty())


cloud-fan · 2020-08-27T07:35:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

@@ -40,9 +41,12 @@ class UnresolvedException[TreeType <: TreeNode[_]](tree: TreeType, function: Str
 * Holds the name of a relation that has yet to be looked up in a catalog.
 *
 * @param multipartIdentifier table name
+ * @param options options for this relation


options to scan this relation. Only applicable to v2 table scan.

cloud-fan · 2020-08-27T07:36:29Z

LGTM except 2 minor comments

SparkQA · 2020-08-27T16:06:58Z

Test build #127959 has finished for PR 29535 at commit a50bf2f.

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

huaxingao · 2020-08-27T16:31:52Z

retest this please

viirya

Hm, we don't have test for this change?

SparkQA · 2020-08-27T16:41:22Z

Test build #127961 has finished for PR 29535 at commit a50bf2f.

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

cloud-fan · 2020-08-27T16:44:02Z

it will be tested in the JDBC v2 PR, but yea, better to have a UT here with a fake v2 source.

viirya · 2020-08-27T16:52:13Z

@huaxingao You may need to sync with master.

SparkQA · 2020-08-27T17:19:25Z

Test build #127962 has finished for PR 29535 at commit dcc88ca.

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

viirya · 2020-08-27T17:37:56Z

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

@@ -40,7 +41,6 @@ import org.apache.spark.sql.execution.datasources.csv._
 import org.apache.spark.sql.execution.datasources.jdbc._
 import org.apache.spark.sql.execution.datasources.json.TextInputJsonDataSource
 import org.apache.spark.sql.execution.datasources.v2.{DataSourceV2Relation, DataSourceV2Utils}
-import org.apache.spark.sql.internal.SQLConf


Oh, @huaxingao, don't remove this. Other change uses SQLConf.

SparkQA · 2020-08-27T23:53:10Z

Test build #127965 has finished for PR 29535 at commit f321560.

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

cloud-fan · 2020-08-28T04:25:35Z

We need to fix explain-aqe.sql

huaxingao · 2020-08-28T05:30:07Z

For this explain formatted

EXPLAIN FORMATTED CREATE VIEW explain_view AS SELECT key, val FROM explain_temp1

Now it has

......

(3) UnresolvedRelation
Arguments: [explain_temp1], org.apache.spark.sql.util.CaseInsensitiveStringMap@1f

......

Do we just need to change the expected output to let the test pass? Or we need to fix the code to make the explain string to have more meaningful info than org.apache.spark.sql.util.CaseInsensitiveStringMap@1f?

cloud-fan · 2020-08-28T05:35:45Z

@huaxingao good point. I think we should fix UnresolvedRelation to have a better format, by overriding verboseStringWithOperatorId

huaxingao · 2020-08-28T07:17:38Z

I didn't run the whole sql test suites on my local. I will just take chance and test this on Jenkins. If the change causes other problems, I will fix tomorrow.

SparkQA · 2020-08-28T11:51:27Z

Test build #127988 has finished for PR 29535 at commit 12d90bd.

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

cloud-fan · 2020-08-28T12:00:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

@@ -544,6 +545,8 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
    case None => Nil
    case Some(null) => Nil
    case Some(any) => any :: Nil
+    case map: CaseInsensitiveStringMap =>
+      truncatedString(map.entrySet().toArray(), "[", ", ", "]", maxFields) :: Nil


are you sure this returns meaningful string?

and we should at least display the original map (keys are not lowercased)

we can use asCaseSensitiveMap call.

I tried the following

scala> Map("a" -> 1).asJava.entrySet.toArray.head res1: Object = scala.collection.convert.Wrappers$MapWrapper$$anon$1$$anon$5$$anon$6@60

I think it's safer to generate the string key=value manually.

I tried this:

scala> val options = Map( | "Key1" -> "Value1", | "key2" -> "vaue2" | ) options: scala.collection.immutable.Map[String,String] = Map(Key1 -> Value1, key2 -> vaue2) scala> val map = new CaseInsensitiveStringMap(options.asJava) map: org.apache.spark.sql.util.CaseInsensitiveStringMap = org.apache.spark.sql.util.CaseInsensitiveStringMap@9f174175 scala> map.asCaseSensitiveMap().entrySet().toArray() res3: Array[Object] = Array(Key1=Value1, key2=vaue2)

I used case map: CaseInsensitiveStringMap for pattern matching, I guess we are probably OK?

Can we add a test case in ExplainSuite?

Sure. Will do. Have a good night!

Seems we can't have a UnresolvedRelation with a non empty CaseInsensitiveStringMap in explain? The only way to create a UnresolvedRelation with a non empty CaseInsensitiveStringMap is using DataFrameReader.table, but this is resolved to a V2Relation.

df.explain(true) will explain the parsed plan, which contains UnresolvedRelation

added a test. Thank you!

SparkQA · 2020-08-31T07:05:01Z

Test build #128077 has finished for PR 29535 at commit b371912.

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

SparkQA · 2020-08-31T07:05:02Z

Test build #128078 has finished for PR 29535 at commit a41c06a.

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

cloud-fan · 2020-08-31T07:42:25Z

retest this please

SparkQA · 2020-08-31T13:09:36Z

Test build #128084 has finished for PR 29535 at commit a41c06a.

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

cloud-fan · 2020-08-31T13:21:12Z

thanks, merging to master!

huaxingao · 2020-08-31T16:02:39Z

Thank you very much! @cloud-fan @viirya

viirya

lgtm too.

HyukjinKwon · 2021-09-23T04:30:38Z

wait, I get confused here. We already defined a table with options. How does it work with the newly set options? are they merged?

cloud-fan · 2021-09-23T04:43:27Z

it's table properties vs scan options

HyukjinKwon · 2021-09-23T04:44:44Z

So UnresolvedReleation is shared for both cases but conditionally use the UnresolvedReleation.options only for Scan? that's very confusing.

HyukjinKwon · 2021-09-23T04:45:18Z

this creates a myth that setting options will overwrite table properties. see also #34072

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

cloud-fan · 2021-09-23T05:06:23Z

this creates a myth that setting options will overwrite table properties.

This is expected. Per-scan options have higher priority than table properties.

HyukjinKwon · 2021-09-23T05:15:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

@@ -40,9 +41,12 @@ class UnresolvedException[TreeType <: TreeNode[_]](tree: TreeType, function: Str
 * Holds the name of a relation that has yet to be looked up in a catalog.
 *
 * @param multipartIdentifier table name
+ * @param options options to scan this relation. Only applicable to v2 table scan.


okay, I just noticed 5e82548 added the merging behaviour for V1.

Okay, maybe we should fix the comments here.

I will have a followup to fix the comment.

opened a PR #34075

Oh, I created a PR without seeing your comment. Thanks @huaxingao.

…ation ### What changes were proposed in this pull request? This PR fixes the 'options' description on `UnresolvedRelation`. This comment was added in #29535 but not valid anymore because V1 also uses this `options` (and merge the options with the table properties) per #29712. This PR can go through from `master` to `branch-3.1`. ### Why are the changes needed? To make `UnresolvedRelation.options`'s description clearer. ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? Scala linter by `dev/linter-scala`. Closes #34075 from HyukjinKwon/minor-comment-unresolved-releation. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Huaxin Gao <huaxin_gao@apple.com>

…ation ### What changes were proposed in this pull request? This PR fixes the 'options' description on `UnresolvedRelation`. This comment was added in #29535 but not valid anymore because V1 also uses this `options` (and merge the options with the table properties) per #29712. This PR can go through from `master` to `branch-3.1`. ### Why are the changes needed? To make `UnresolvedRelation.options`'s description clearer. ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? Scala linter by `dev/linter-scala`. Closes #34075 from HyukjinKwon/minor-comment-unresolved-releation. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Huaxin Gao <huaxin_gao@apple.com> (cherry picked from commit 0076eba) Signed-off-by: Huaxin Gao <huaxin_gao@apple.com>

probot-autolabeler bot added the SQL label Aug 25, 2020

cloud-fan reviewed Aug 25, 2020

View reviewed changes

cloud-fan reviewed Aug 27, 2020

View reviewed changes

viirya reviewed Aug 27, 2020

View reviewed changes

cloud-fan reviewed Aug 27, 2020

View reviewed changes

viirya reviewed Aug 27, 2020

View reviewed changes

huaxingao added 5 commits August 27, 2020 10:02

[SPARK-32592][SQL] Make DataFrameReader.table take the specified options

59971c4

fix

04cf5a1

fix

2d1e0f9

address comments

5567b7c

address comments

dcc88ca

huaxingao force-pushed the table_options branch from a50bf2f to dcc88ca Compare August 27, 2020 17:05

viirya reviewed Aug 27, 2020

View reviewed changes

fix test failure

12d90bd

cloud-fan reviewed Aug 28, 2020

View reviewed changes

huaxingao added 2 commits August 30, 2020 23:41

add test in ExplainSuite

b371912

nit

a41c06a

cloud-fan closed this in 806140d Aug 31, 2020

huaxingao deleted the table_options branch August 31, 2020 16:02

viirya reviewed Aug 31, 2020

View reviewed changes

HyukjinKwon reviewed Sep 23, 2021

View reviewed changes

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

HyukjinKwon reviewed Sep 23, 2021

View reviewed changes

HyukjinKwon mentioned this pull request Sep 23, 2021

[MINOR][SQL][DOCS] Correct the 'options' description on UnresolvedRelation #34075

Closed

		@@ -42,7 +42,9 @@ class UnresolvedException[TreeType <: TreeNode[_]](tree: TreeType, function: Str
		* @param multipartIdentifier table name

[SPARK-32592][SQL] Make DataFrameReader.table take the specified options #29535

[SPARK-32592][SQL] Make DataFrameReader.table take the specified options #29535

Conversation

huaxingao commented Aug 25, 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?

SparkQA commented Aug 25, 2020

huaxingao commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 26, 2020

SparkQA commented Aug 27, 2020

huaxingao commented Aug 27, 2020

Choose a reason for hiding this comment

cloud-fan Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaxingao commented Aug 27, 2020

SparkQA commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Aug 27, 2020

SparkQA commented Aug 27, 2020

huaxingao commented Aug 27, 2020

viirya left a comment

Choose a reason for hiding this comment

SparkQA commented Aug 27, 2020

cloud-fan commented Aug 27, 2020

viirya commented Aug 27, 2020

SparkQA commented Aug 27, 2020

viirya Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Aug 27, 2020

cloud-fan commented Aug 28, 2020

huaxingao commented Aug 28, 2020

cloud-fan commented Aug 28, 2020

huaxingao commented Aug 28, 2020

SparkQA commented Aug 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 31, 2020

SparkQA commented Aug 31, 2020

cloud-fan commented Aug 31, 2020

SparkQA commented Aug 31, 2020

cloud-fan commented Aug 31, 2020

huaxingao commented Aug 31, 2020

viirya left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 23, 2021

cloud-fan commented Sep 23, 2021

HyukjinKwon commented Sep 23, 2021

HyukjinKwon commented Sep 23, 2021

cloud-fan commented Sep 23, 2021

HyukjinKwon Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

cloud-fan Aug 26, 2020 •

edited

Loading

cloud-fan Aug 27, 2020 •

edited

Loading

viirya Aug 27, 2020 •

edited

Loading

HyukjinKwon Sep 23, 2021 •

edited

Loading

HyukjinKwon Sep 23, 2021 •

edited

Loading