New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-37330][SQL] Migrate ReplaceTableStatement to v2 command #34764
Conversation
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #145795 has finished for PR 34764 at commit
|
9947a2e
to
66a2aaf
Compare
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #145836 has finished for PR 34764 at commit
|
66a2aaf
to
ec966bf
Compare
Kubernetes integration test starting |
CC @cloud-fan @Peng-Lei @huaxingao , Thanks! |
Kubernetes integration test status failure |
Test build #145848 has finished for PR 34764 at commit
|
LGTM, But I just think @huaxingao maybe is planning to do this issue. because it is very similar with |
@@ -3563,9 +3563,12 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg | |||
case _ => | |||
// Note: table schema includes both the table columns list and the partition columns | |||
// with data type. | |||
val tableSpec = TableSpec(bucketSpec, properties, provider, options, location, comment, | |||
serdeInfo, external) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to move this out of the pattern matching block so it can be used by both ReplaceTableAsSelect
and ReplaceTable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks!
Kubernetes integration test starting |
Kubernetes integration test status failure |
options, location, comment, serdeInfo, orCreate = orCreate) | ||
ReplaceTable( | ||
UnresolvedDBObjectName(table, isNamespace = false), | ||
schema, partitioning, tableSpec.copy(external = external), orCreate = orCreate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think external
is always false. https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala#L3526
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think external
is outdated. Today Spark have no key word EXTERNAL
for CREATE/REPLACE TABLE
. We use LOCATION
to diff the external table
and managed table
to unify the syntax for create data souce table
. So maybe #Line need to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you all,
I think remove external checking code should be in other PR, right?
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #145871 has finished for PR 34764 at commit
|
Test build #145882 has finished for PR 34764 at commit
|
sorry there are code conflicts now |
f3cca49
to
05d5a41
Compare
Test build #145904 has finished for PR 34764 at commit
|
Kubernetes integration test unable to build dist. exiting with code: 1 |
}.getOrElse(props) | ||
val propsWithOwner = CatalogV2Util.withDefaultOwnership(newProps) | ||
case ReplaceTable(ResolvedDBObjectName(catalog, ident), schema, parts, tableSpec, orCreate) => | ||
val qualifiedLocation = tableSpec.location.map(makeQualifiedDBObjectPath(_)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR: seems path is not qualified in CreateTableAsSelect
? cc @imback82
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it doesn't seem to be qualified. I will create a PR for that.
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #145908 has finished for PR 34764 at commit
|
@dchvn Could you rebase? I know the Git Action failures are not related to this PR, but with so many failures, I don't feel comfortable to merge the PR. |
Ping @huaxingao , GitHub action is good now, thanks |
Merged to master. Thanks! |
many thanks! @huaxingao |
What changes were proposed in this pull request?
This PR migrates ReplaceTableStatement to the v2 command
Why 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