[SPARK-32844][SQL] Make DataFrameReader.table take the specified options for datasource v1
#29712
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xuanyuanking
changed the title
DataFrameReader.table take the specified options for datasource v1DataFrameReader.table take the specified options for datasource v1
|
cc @cloud-fan |
cloud-fan
reviewed
Sep 10, 2020
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 10, 2020
| @@ -251,24 +255,25 @@ class FindDataSourceTable(sparkSession: SparkSession) extends Rule[LogicalPlan] | |||
| partitionColumns = table.partitionColumnNames, | |||
| bucketSpec = table.bucketSpec, | |||
| className = table.provider.get, | |||
| options = table.storage.properties ++ pathOption, | |||
| options = extraOptions.asCaseSensitiveMap.asScala.toMap | |||
| ++ table.storage.properties ++ pathOption, | |||
There was a problem hiding this comment.
nit: let's say table properties have key=1, and extraOptions have KEY=1. The check can pass, but the final options have both key and KEY. We should avoid it and keeping key from table properties.
There was a problem hiding this comment.
Thanks, done in cc250b9.
|
Test build #128511 has finished for PR 29712 at commit
|
|
Test build #128518 has finished for PR 29712 at commit
|
HyukjinKwon
reviewed
Sep 11, 2020
| @@ -190,4 +192,15 @@ object DataSourceUtils { | |||
| case LegacyBehaviorPolicy.LEGACY => RebaseDateTime.rebaseGregorianToJulianMicros | |||
| case LegacyBehaviorPolicy.CORRECTED => identity[Long] | |||
| } | |||
|
|
|||
| private[sql] def checkDuplicateOptions( | |||
There was a problem hiding this comment.
We can remove private[sql] since execution package is already private (SPARK-16964)
There was a problem hiding this comment.
Ah yeah, thanks for reminding, done in cc250b9.
HyukjinKwon
reviewed
Sep 11, 2020
sql/core/src/test/scala/org/apache/spark/sql/test/DataFrameReaderWriterSuite.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Sep 11, 2020
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
Outdated
Show resolved
Hide resolved
|
Test build #128564 has finished for PR 29712 at commit
|
|
Test build #128565 has finished for PR 29712 at commit
|
xuanyuanking
force-pushed
the
SPARK-32844
branch
from
05aa245 to
f3f8c8d
Compare
|
Test build #128579 has finished for PR 29712 at commit
|
|
thanks, merging to master! |
|
Thanks for the review. |
huaxingao
pushed a commit
that referenced
this pull request
Sep 23, 2021
…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>
huaxingao
pushed a commit
that referenced
this pull request
Sep 23, 2021
…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>
huaxingao
pushed a commit
that referenced
this pull request
Sep 23, 2021
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Make
DataFrameReader.tabletake the specified options for datasource v1.Why are the changes needed?
Keep the same behavior of v1/v2 datasource, the v2 fix has been done in SPARK-32592.
Does this PR introduce any user-facing change?
Yes. The DataFrameReader.table will take the specified options. Also, if there are the same key and value exists in specified options and table properties, an exception will be thrown.
How was this patch tested?
New UT added.