[SPARK-52272][SQL] V2SessionCatalog does not alter schema on Hive Catalog #51007
Conversation
| catalogTable.copy( | ||
| properties = finalProperties, schema = schema, owner = owner, comment = comment, | ||
| collation = collation, storage = storage)) | ||
| if (SQLConf.get.getConf(StaticSQLConf.CATALOG_IMPLEMENTATION).equals("hive")) { |
There was a problem hiding this comment.
Note: SessionCatalog (V1) alterTableDataSchema explicitly does not support 'drop column'. https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala#L499
So to avoid regression in InMemoryCatalog, in which alterTable can alter the schema including 'drop column', I make the change only for "hive". Following the config check for CATALOG_IMPLEMENTATION in other parts of the code.
There was a problem hiding this comment.
Does it break any test? InMemoryCatalog is for testing and I think it's OK to always call v1 alterTableDataSchema for table schema changes.
There was a problem hiding this comment.
let me check if it breaks tests.
| @@ -3392,4 +3397,180 @@ class HiveDDLSuite | |||
| ) | |||
| } | |||
| } | |||
|
|
|||
| test("SPARK-52272: V2SessionCatalog does not alter schema on Hive Catalog") { | |||
| val externalCatalog = new CustomHiveCatalog(spark.sessionState.catalog.externalCatalog) | |||
There was a problem hiding this comment.
why do we need a CustomHiveCatalog to test it? I think we can get the real v2 session catalog via session.sessionState.catalogManager.v2SessionCatalog and test it.
There was a problem hiding this comment.
yea sorry it was late :), i rewrote it using mockito. I needed something to check which methods are called.
| properties = finalProperties, schema = schema, owner = owner, comment = comment, | ||
| collation = collation, storage = storage)) | ||
| } | ||
| if (changes.exists(_.isInstanceOf[TableChange.ColumnChange])) { |
There was a problem hiding this comment.
The code assumes that the table changes are either all column changes, or all non-column changes. We should either use assert to guarantee this assumption, or not make this assumption: call both alterTable and alterTableDataSchema if both column and non-column changes are present.
There was a problem hiding this comment.
hm, yea i think this code does call both, if both are present?
Seems a v2 test is testing an unsupported feature. Shall we remove it? |
…essionCatalog due to V2 sessionCatalog limitations
|
thanks, merging to master! |
What changes were proposed in this pull request?
V2SessionCatalog delegates alterTable to SessionCatalog (V1) alterTable. In the case of "hive", this should be changed to delegate to either alterTable or alterTableDataSchema.
Why are the changes needed?
SessionCatalog has two API's, alterTable and alterTableDataSchema.
alterTable will silently ignore schema changes in Hive implementation (the API Javadoc:
If the underlying implementation does not support altering a certain field, this becomes a no-op.).So for Hive case, V2SessionCatalog needs to delegate schema changes to V1 alterTableDataSchema
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add new test in HiveDDLSuite, verify functionality and which method is called.
Was this patch authored or co-authored using generative AI tooling?
No