[GitHub] [spark] rdblue commented on a change in pull request #25502: [SPARK-28668][SQL] Support V2SessionCatalog for ALTER TABLE

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #25502: [SPARK-28668][SQL] Support V2SessionCatalog for ALTER TABLE
URL: https://github.com/apache/spark/pull/25502#discussion_r319300747
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
 ##########
 @@ -922,51 +916,51 @@ class Analyzer(
           TableChange.updateColumnComment(colName.toArray, newComment)
         }
 
-        AlterTable(
-          v2Catalog.asTableCatalog, ident,
-          UnresolvedRelation(alter.tableName),
-          typeChange.toSeq ++ commentChange.toSeq)
+        resolveV2Alter(tableName, typeChange.toSeq ++ commentChange.toSeq).getOrElse(alter)
 
-      case alter @ AlterTableRenameColumnStatement(
-          CatalogObjectIdentifier(Some(v2Catalog), ident), col, newName) =>
-        AlterTable(
-          v2Catalog.asTableCatalog, ident,
-          UnresolvedRelation(alter.tableName),
-          Seq(TableChange.renameColumn(col.toArray, newName)))
+      case alter @ AlterTableRenameColumnStatement(tableName, col, newName) =>
+        val changes = Seq(TableChange.renameColumn(col.toArray, newName))
+        resolveV2Alter(tableName, changes).getOrElse(alter)
 
-      case alter @ AlterTableDropColumnsStatement(
-          CatalogObjectIdentifier(Some(v2Catalog), ident), cols) =>
+      case alter @ AlterTableDropColumnsStatement(tableName, cols) =>
         val changes = cols.map(col => TableChange.deleteColumn(col.toArray))
-        AlterTable(
-          v2Catalog.asTableCatalog, ident,
-          UnresolvedRelation(alter.tableName),
-          changes)
-
-      case alter @ AlterTableSetPropertiesStatement(
-          CatalogObjectIdentifier(Some(v2Catalog), ident), props) =>
-        val changes = props.map {
-          case (key, value) =>
-            TableChange.setProperty(key, value)
+        resolveV2Alter(tableName, changes).getOrElse(alter)
+
+      case alter @ AlterTableSetPropertiesStatement(tableName, props) =>
+        val changes = props.map { case (key, value) =>
+          TableChange.setProperty(key, value)
         }
 
-        AlterTable(
-          v2Catalog.asTableCatalog, ident,
-          UnresolvedRelation(alter.tableName),
-          changes.toSeq)
-
-      case alter @ AlterTableUnsetPropertiesStatement(
-          CatalogObjectIdentifier(Some(v2Catalog), ident), keys, _) =>
-        AlterTable(
-          v2Catalog.asTableCatalog, ident,
-          UnresolvedRelation(alter.tableName),
-          keys.map(key => TableChange.removeProperty(key)))
-
-      case alter @ AlterTableSetLocationStatement(
-          CatalogObjectIdentifier(Some(v2Catalog), ident), newLoc) =>
-        AlterTable(
-          v2Catalog.asTableCatalog, ident,
-          UnresolvedRelation(alter.tableName),
-          Seq(TableChange.setProperty("location", newLoc)))
+        resolveV2Alter(tableName, changes.toSeq).getOrElse(alter)
+
+      case alter @ AlterTableUnsetPropertiesStatement(tableName, keys, _) =>
+        resolveV2Alter(tableName, keys.map(key => TableChange.removeProperty(key))).getOrElse(alter)
+
+      case alter @ AlterTableSetLocationStatement(tableName, newLoc) =>
+        resolveV2Alter(tableName, Seq(TableChange.setProperty("location", newLoc))).getOrElse(alter)
+    }
+
+    private def resolveV2Alter(
+        tableName: Seq[String],
+        changes: Seq[TableChange]): Option[AlterTable] = {
 
 Review comment:
   I understand the motivation to create the `lookupV2Relation` method, but I'm don't quite like the effects that it has on these rules because it mixes identifier resolution (which catalog is responsible?) with table resolution (look up the table in a catalog). This leads to some strange changes, like the addition of `AlterTableStatement` to `checkAnalysis` that throws "Table or view not found" when the real problem is that the statement wasn't converted to a v1 or v2 plan.
   
   Before this change, only identifier resolution is done. Table resolution is done by the `ResolveTables` rule so it is fairly well contained. Plans were created with a place-holder `UnresolvedRelation`, which avoids the need to catch `AlterTableStatement` in `checkAnalysis`
   
   But, to add the fallback to v1 for tables that are loaded by the v2 session catalog, we have to look up the table and only convert to the v2 plan if it isn't an `UnresolvedTable`.
   
   I'm not sure what the _right_ solution is. Maybe instead of avoiding conversion to the v2 plan, we should convert from v2 to v1 for the fallback case. I think that would make the rules cleaner and more orthogonal to one another.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org