ulysses-you commented on a change in pull request #25747: [SPARK-29039][SQL]
centralize the catalog and table lookup logic
URL: https://github.com/apache/spark/pull/25747#discussion_r354658565
##########
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala
##########
@@ -642,51 +678,166 @@ class PlanResolutionSuite extends AnalysisTest {
// ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment);
// ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key');
test("alter table: alter table properties") {
- val sql1_table = "ALTER TABLE table_name SET TBLPROPERTIES ('test' =
'test', " +
- "'comment' = 'new_comment')"
- val sql2_table = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment',
'test')"
- val sql3_table = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS
('comment', 'test')"
+ Seq("v1Table" -> true, "v2Table" -> false, "testcat.tab" -> false).foreach
{
+ case (tblName, useV1Command) =>
+ val sql1 = s"ALTER TABLE $tblName SET TBLPROPERTIES ('test' = 'test',
" +
+ "'comment' = 'new_comment')"
+ val sql2 = s"ALTER TABLE $tblName UNSET TBLPROPERTIES ('comment',
'test')"
+ val sql3 = s"ALTER TABLE $tblName UNSET TBLPROPERTIES IF EXISTS
('comment', 'test')"
+
+ val parsed1 = parseAndResolve(sql1)
+ val parsed2 = parseAndResolve(sql2)
+ val parsed3 = parseAndResolve(sql3)
+
+ val tableIdent = TableIdentifier(tblName, None)
+ if (useV1Command) {
+ val expected1 = AlterTableSetPropertiesCommand(
+ tableIdent, Map("test" -> "test", "comment" -> "new_comment"),
isView = false)
+ val expected2 = AlterTableUnsetPropertiesCommand(
+ tableIdent, Seq("comment", "test"), ifExists = false, isView =
false)
+ val expected3 = AlterTableUnsetPropertiesCommand(
+ tableIdent, Seq("comment", "test"), ifExists = true, isView =
false)
+
+ comparePlans(parsed1, expected1)
+ comparePlans(parsed2, expected2)
+ comparePlans(parsed3, expected3)
+ } else {
+ parsed1 match {
+ case AlterTable(_, _, _: DataSourceV2Relation, changes) =>
+ assert(changes == Seq(
+ TableChange.setProperty("test", "test"),
+ TableChange.setProperty("comment", "new_comment")))
+ case _ => fail("expect AlterTable")
+ }
+
+ parsed2 match {
+ case AlterTable(_, _, _: DataSourceV2Relation, changes) =>
+ assert(changes == Seq(
+ TableChange.removeProperty("comment"),
+ TableChange.removeProperty("test")))
+ case _ => fail("expect AlterTable")
+ }
+
+ parsed3 match {
+ case AlterTable(_, _, _: DataSourceV2Relation, changes) =>
+ assert(changes == Seq(
+ TableChange.removeProperty("comment"),
+ TableChange.removeProperty("test")))
+ case _ => fail("expect AlterTable")
+ }
+ }
+ }
- val parsed1_table = parseAndResolve(sql1_table)
- val parsed2_table = parseAndResolve(sql2_table)
- val parsed3_table = parseAndResolve(sql3_table)
+ val sql4 = "ALTER TABLE non_exist SET TBLPROPERTIES ('test' = 'test')"
+ val sql5 = "ALTER TABLE non_exist UNSET TBLPROPERTIES ('test')"
+ val parsed4 = parseAndResolve(sql4)
+ val parsed5 = parseAndResolve(sql5)
- val tableIdent = TableIdentifier("table_name", None)
- val expected1_table = AlterTableSetPropertiesCommand(
- tableIdent, Map("test" -> "test", "comment" -> "new_comment"), isView =
false)
- val expected2_table = AlterTableUnsetPropertiesCommand(
- tableIdent, Seq("comment", "test"), ifExists = false, isView = false)
- val expected3_table = AlterTableUnsetPropertiesCommand(
- tableIdent, Seq("comment", "test"), ifExists = true, isView = false)
-
- comparePlans(parsed1_table, expected1_table)
- comparePlans(parsed2_table, expected2_table)
- comparePlans(parsed3_table, expected3_table)
+ // For non-existing tables, we convert it to v2 command with
`UnresolvedV2Table`
Review comment:
I do agree v2 command is better than v1. But it will make some things
unexpected. when load table return none, `ResolveSessionCatalog` will check if
v2 support it, and throw not support msg when not support and throw table not
found when support. I do not determine which msg is the high priority. Here is
a example:
```
create table test(c int);
// throw Table not found
desc tes;
// throw Describing columns is not supported for v2 tables.;
desc tes c;
```
Shall we keep this consistent when table is non-existing ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
