[GitHub] [iceberg] pvary commented on pull request #1505: Hive: Make HiveCatalog based tables automatically readable from Hive

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

pvary commented on pull request #1505:
URL: https://github.com/apache/iceberg/pull/1505#issuecomment-703850831


   > > Spark is using a v1 command to drop the table: `org.apache.spark.sql.execution.command.DropTableCommand`. That is unexpected because the test should be using an Iceberg implementation of Spark's `TableCatalog` for all of these operations. Can you find out what catalog was being used? My guess is that it was the built-in [`spark_catalog`](https://github.com/apache/iceberg/blob/1772f4f27b8a12d3e89a7f65b8b600b717e1f09d/spark3/src/test/java/org/apache/iceberg/spark/SparkCatalogTestBase.java#L66-L71) and that somehow the session catalog wrapper that we injected did not correctly detect that this table is Iceberg and not Hive.
   
   I guess this is a spark bug.
   
   What I see is that the spark_catalog is correctly set (CatalogManager.defaultSessionCatalog is the original inner HiveSessionCatalog, but the CatalogManager.catalogs has a value for spark_catalog pointing to our very own SparkSessionCatalog).
   Sadly this code does not trigger DropTable command creation during the planning:
   ```
       case DropTableStatement(NonSessionCatalogAndTable(catalog, tbl), ifExists, _) =>
         DropTable(catalog.asTableCatalog, tbl.asIdentifier, ifExists)
   ```
   https://github.com/apache/spark/blob/v3.0.0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala#L175-L176
   
   I know from debugging that the NonSessionCatalogAndTable is None (since with spark_catalog we replace session catalog so it is not a NonSessionCatalogAndTable), so I guess we do not translate the DropTableStatement to DropTable.
   We fall back to ResolvaSessionCatalog and convert the statement to DropTableCommand which does not care about the catalog
   ```
       case DropTableStatement(TempViewOrV1Table(name), ifExists, purge) =>
         DropTableCommand(name.asTableIdentifier, ifExists, isView = false, purge = purge)
   ```
   https://github.com/apache/spark/blob/v3.0.0/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala#L331-L332
   
   My theory is that we are missing the checks and different code paths for V2 providers which we have for CreateTableStatement for example:
   ```
         if (!isV2Provider(provider)) {
   [..]
         } else {
   [..]
         }
   ```
   https://github.com/apache/spark/blob/v3.0.0/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala#L248
   
   So to summarize the things again:
   - This is a Spark bug, where the `ResolveSessionCatalog` should use different code for V1 and V2 providers when a DropTableStatement is issued
   
   @rdblue: I am not an expert in Scala, so happy to hear any objections if I made mistakes. Does my analysis makes sense? How should we proceed?
   
   Thanks,
   Peter 


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org