Uh oh!
There was an error while loading. Please reload this page.
[SPARK-37496][SQL] Migrate ReplaceTableAsSelectStatement to v2 command - #34754
[SPARK-37496][SQL] Migrate ReplaceTableAsSelectStatement to v2 command#34754huaxingao wants to merge 2 commits into
Conversation
SparkQA
commented
Nov 30, 2021
Kubernetes integration test starting |
SparkQA
commented
Nov 30, 2021
Kubernetes integration test status failure |
SparkQA
commented
Nov 30, 2021
Test build #145757 has finished for PR 34754 at commit
|
huaxingao
commented
Nov 30, 2021
cc @cloud-fan Could you please take a look? |
| extraOptions.toMap, | ||
| None, | ||
| orCreate = true) // Create the table if it doesn't exist | ||
| val tableSpec = TableSpec(None, Map.empty, Some(source), Map.empty, |
There was a problem hiding this comment.
let's use named parameter here: TableSpec(bucketSpec = None, ...)
| options.toMap, | ||
| None, | ||
| orCreate = orCreate)) | ||
| val tableSpec = TableSpec(None, properties.toMap, provider, Map.empty, |
| invalidateCache: (TableCatalog, Table, Identifier) => Unit) extends TableWriteExecHelper { | ||
| val properties = { | ||
| val props = CatalogV2Util.convertTableProperties( |
There was a problem hiding this comment.
nit: should the util function take TableSpec directly? and also fill the ownership property. Then we can simplify the code here and other places.
SparkQA
commented
Dec 1, 2021
Kubernetes integration test starting |
| serde = None, | ||
| external = false) | ||
| ReplaceTableAsSelect( | ||
| UnresolvedDBObjectName(nameParts, isNamespace = false), |
There was a problem hiding this comment.
shall we prepend catalog.name?
There was a problem hiding this comment.
Seems no need? I stepped into one of the test, the nameParts here is "testcat", "ns", "t", so it already contains catalog.name.
SparkQA
commented
Dec 1, 2021
Kubernetes integration test status failure |
SparkQA
commented
Dec 1, 2021
Test build #145820 has finished for PR 34754 at commit
|
huaxingao
commented
Dec 1, 2021
Merged to master. Thank you very much for reviewing! @cloud-fan |
| orCreate = true) // Create the table if it doesn't exist | ||
| df.queryExecution.analyzed, | ||
| tableSpec, | ||
| writeOptions = Map.empty, |
There was a problem hiding this comment.
I think there is a bug here. Previously we pass extraOptions.toMap as the write options, now we don't. @huaxingao can you help to fix it with a test case? thanks!
There was a problem hiding this comment.
Sorry for the bug. Will fix.
…write options in CTAS and RTAS ### What changes were proposed in this pull request? `DataFrameWriter.saveAsTable` should pass `extraOptions.toMap` as the write options in CTAS and RTAS ### Why are the changes needed? bug fixing please see #34754 (comment)#34667 (comment) ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new test Closes#34997 from huaxingao/write_option. Authored-by: Huaxin Gao <huaxin_gao@apple.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
This PR migrates
ReplaceTableAsSelectStatementto the v2 commandWhy are the changes needed?
Migrate to the standard V2 framework
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing tests