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:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
