[SPARK-56611][SQL] Fix ALTER TABLE RENAME TO with catalog-qualified and table-only targets#55547
[SPARK-56611][SQL] Fix ALTER TABLE RENAME TO with catalog-qualified and table-only targets#55547lunar-shadow wants to merge 3 commits intoapache:masterfrom
Conversation
669a517 to
8163d49
Compare
…nd table-only targets
8163d49 to
cf3a248
Compare
|
|
||
| // Strip catalog prefix if the identifier is catalog-qualified. | ||
| val newNameParts = | ||
| if (rawNewNameParts.length > 1 && rawNewNameParts.head == catalog.name()) { |
There was a problem hiding this comment.
This looks like case-sensitive for catalog names which is incorrect, doesn't it? Please add a unit test case like the following?
ALTER TABLE testcat.ns.t1 RENAME TO TesTcaT.ns.t1_renamed
There was a problem hiding this comment.
Thanks @dongjoon-hyun for catching this.
Fixed, now using SQLConf.get.resolver for case-insensitive comparison. Added a test case: ALTER TABLE testcat.ns.t1_renamed RENAME TO TesTcaT.ns.t1
| } | ||
|
|
||
| val namespace = | ||
| if (newNameParts.length == 1) { |
There was a problem hiding this comment.
Please follow the community Scala coding style for val namespace = if (...) {.
val ret = if (condition) {
Seq(1, 2, 3)
} else {
Seq(1, 2, 3)
}
| } | ||
| } | ||
|
|
||
| test("SPARK-56611: rename table across namespace should work for V2 catalogs") { |
There was a problem hiding this comment.
We don't need to mention for V2 catalogs phrase because this is inside DataSourceV2SQLSuite.
There was a problem hiding this comment.
Done, renamed to "rename table across namespaces"
| if (newNameParts.length == 1) { | ||
| oldIdent.namespace() | ||
| } else { | ||
| newNameParts.dropRight(1).toArray |
There was a problem hiding this comment.
In this case, we can use init.toArray like asIdentifier.
- newNameParts.dropRight(1).toArray
+ newNameParts.init.toArray
| sql("CREATE TABLE testcat.ns1.t1 USING foo AS SELECT id, data FROM source") | ||
| checkAnswer(sql("SHOW TABLES FROM testcat.ns1"), Seq(Row("ns1", "t1", false))) | ||
|
|
||
| sql("ALTER TABLE testcat.ns1.t1 RENAME TO ns2.t1") |
There was a problem hiding this comment.
Dose this PR aim to support ALTER TABLE testcat.ns1.t1 RENAME TO othercat.ns2.t1? Could you add a test case?
There was a problem hiding this comment.
Cross-catalog rename is not addressed as part of this PR. I noticed it currently silently treats the other catalog name as a namespace. I'll file a follow-up JIRA for proper validation.
There was a problem hiding this comment.
Shouldn't the above be interpreted as move (rename) t1 table from ns1 namespace to othercat.ns2 namespace in testcat catalog? So doesn't the current PR work as expected?
There was a problem hiding this comment.
Right, cross-catalog rename may not be a valid operation as RENAME operates within a single catalog. So othercat.ns2.t1 is correctly treated as a namespace path within testcat. Added a test case to verify this. No follow-up needed, thanks for the clarification.
| if (rawNewNameParts.length > 1 && rawNewNameParts.head == catalog.name()) { | ||
| rawNewNameParts.tail | ||
| } else { | ||
| rawNewNameParts |
There was a problem hiding this comment.
Do we have a test case for this?
There was a problem hiding this comment.
Yes, both RENAME TO testcat.ns.t1_renamed and RENAME TO TesTcaT.ns.t1 in the test hit this code path.
|
cc @peter-toth , too. |
…s review feedback
| val namespace = if (newNameParts.length == 1) { | ||
| oldIdent.namespace() | ||
| } else { | ||
| newNameParts.init.toArray | ||
| } | ||
|
|
||
| val newIdent = Identifier.of(namespace, newNameParts.last) |
There was a problem hiding this comment.
Minor nit:
| val namespace = if (newNameParts.length == 1) { | |
| oldIdent.namespace() | |
| } else { | |
| newNameParts.init.toArray | |
| } | |
| val newIdent = Identifier.of(namespace, newNameParts.last) | |
| val newIdent = if (newNameParts.length == 1) { | |
| Identifier.of(oldIdent.namespace(), newNameParts.last) | |
| } else { | |
| newNameParts.asIdentifier | |
| } |
There was a problem hiding this comment.
adopted this change, thanks!
| sql("CREATE TABLE testcat.ns1.t1 USING foo AS SELECT id, data FROM source") | ||
| checkAnswer(sql("SHOW TABLES FROM testcat.ns1"), Seq(Row("ns1", "t1", false))) | ||
|
|
||
| sql("ALTER TABLE testcat.ns1.t1 RENAME TO ns2.t1") |
There was a problem hiding this comment.
Shouldn't the above be interpreted as move (rename) t1 table from ns1 namespace to othercat.ns2 namespace in testcat catalog? So doesn't the current PR work as expected?
…alog namespace resolution
What changes were proposed in this pull request?
This fixes
ALTER TABLE ... RENAME TOin V2 catalogs for two cases that weren't handled correctly:Changes:
Why are the changes needed?
Right now, rename behaves incorrectly in a few scenarios.
For example:
fails because
asIdentifiertreats everything except the last part as the namespace. That means the catalog name (testcat) incorrectly ends up inside the namespace:instead of:
Similarly:
creates an identifier with an empty namespace, instead of inheriting
nsfrom the source table.Steps to reproduce:
This fails with a namespace resolution error due to the catalog being treated as part of the namespace.
Does this PR introduce any user-facing change?
Yes.
RENAME TO t2) now correctly keeps the original namespaceHow was this patch tested?
Added tests in
DataSourceV2SQLSuitecovering catalog-qualified renames, table-only renames, and cross-namespace renames.Was this patch authored or co-authored using generative AI tooling?
No