[SPARK-49519][SQL] Merge options of table and relation when constructing FileScanBuilder #47996
Conversation
liujiayi771
force-pushed
the
csv-options
branch
from
201d78d
to
54fd1b1
Compare
|
Hi @cloud-fan. Could you help to review? |
| CSVScanBuilder(sparkSession, fileIndex, schema, dataSchema, options) | ||
| override def newScanBuilder(options: CaseInsensitiveStringMap): CSVScanBuilder = { | ||
| val finalOptions = this.options.asCaseSensitiveMap().asScala.filterNot { | ||
| case (k, _) => options.containsKey(k) |
There was a problem hiding this comment.
do we need this? I think combining two maps will respect the keys in the later map.
There was a problem hiding this comment.
I will remove the filter and combining directly.
| case (k, _) => options.containsKey(k) | ||
| }.toMap ++ options.asScala.toMap | ||
| CSVScanBuilder(sparkSession, fileIndex, schema, dataSchema, | ||
| new CaseInsensitiveStringMap(finalOptions.asJava)) |
There was a problem hiding this comment.
do other formats have the same issue?
There was a problem hiding this comment.
@cloud-fan orc/parquet/json/textTable do not combine the options of table and relation, while jdbcTable does merge them. I think we can add merging logic for all of them. How do you think?
liujiayi771
changed the title
liujiayi771
force-pushed
the
csv-options
branch
from
cf4cdab
to
91752e9
Compare
liujiayi771
force-pushed
the
csv-options
branch
from
91752e9
to
834995b
Compare
|
Is it possible to add a test? |
There was a problem hiding this comment.
As mentioned above, it needs a test case for the following claim. Please add it to protect your contribution from any potential future regressions, @liujiayi771 .
If only the relation options are used here, the TableCatalog will not be able to pass dsOptions that contains table options to FileScan.
|
Okay, I will add a test case, thanks. |
|
@cloud-fan @dongjoon-hyun I have added a test case, could you help to review? |
|
+1, LGTM |
There was a problem hiding this comment.
+1, LGTM. Thank you, @liujiayi771 , @cloud-fan , @adrian-wang .
Merged to master for Apache Spark 4.0.0-preview2.
|
To @liujiayi771 , FYI, Apache Spark 4.0.0-preview2 RC1 will be released next Monday. |
…ing FileScanBuilder ### What changes were proposed in this pull request? Merge the options of both `DataSourceV2Relation` and `FileTable` when constructing `FileScanBuilder`. ### Why are the changes needed? Currently, the subclass of `FileTable` only uses the options from relation when constructing the `FileScanBuilder`, which leads to the omission of the contents in `FileTable.options`. For the `TableCatalog`, the `dsOptions` can be set into the `FileTable.options` returned by the `TableCatalog.loadTable` method. If only the relation options are used here, the `TableCatalog` will not be able to pass `dsOptions` that contains table options to `FileScan`. Merge the two options is a better option. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47996 from liujiayi771/csv-options. Lead-authored-by: joey.ljy <joey.ljy@alibaba-inc.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ing FileScanBuilder ### What changes were proposed in this pull request? Merge the options of both `DataSourceV2Relation` and `FileTable` when constructing `FileScanBuilder`. ### Why are the changes needed? Currently, the subclass of `FileTable` only uses the options from relation when constructing the `FileScanBuilder`, which leads to the omission of the contents in `FileTable.options`. For the `TableCatalog`, the `dsOptions` can be set into the `FileTable.options` returned by the `TableCatalog.loadTable` method. If only the relation options are used here, the `TableCatalog` will not be able to pass `dsOptions` that contains table options to `FileScan`. Merge the two options is a better option. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47996 from liujiayi771/csv-options. Lead-authored-by: joey.ljy <joey.ljy@alibaba-inc.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
Merge the options of both
DataSourceV2RelationandFileTablewhen constructingFileScanBuilder.Why are the changes needed?
Currently, the subclass of
FileTableonly uses the options from relation when constructing theFileScanBuilder, which leads to the omission of the contents inFileTable.options. For theTableCatalog, thedsOptionscan be set into theFileTable.optionsreturned by theTableCatalog.loadTablemethod. If only the relation options are used here, theTableCatalogwill not be able to passdsOptionsthat contains table options toFileScan.Merge the two options is a better option.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No.