laserninja commented on code in PR #10757:
URL: https://github.com/apache/gravitino/pull/10757#discussion_r3231383919


##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java:
##########
@@ -230,12 +230,19 @@ public ConnectorOutputTableHandle beginCreateTable(
       ConnectorTableMetadata tableMetadata,
       Optional<ConnectorTableLayout> layout,
       RetryMode retryMode,
-      boolean noExistingData) {
-    // First, create the table in the Gravitino catalog
+      boolean replace) {
+    SchemaTableName tableName = tableMetadata.getTable();
+
+    // CREATE OR REPLACE TABLE AS SELECT: drop the existing table first if 
present.
+    if (replace
+        && catalogConnectorMetadata.tableExists(
+            tableName.getSchemaName(), tableName.getTableName())) {
+      catalogConnectorMetadata.dropTable(tableName);

Review Comment:
   Thanks for the feedback. The CREATE OR REPLACE TABLE AS SELECT path was 
removed rather than kept with a workaround because of a fundamental 
incompatibility with the Iceberg internal connector's table-identity tracking.
   
   Root cause: The internal Iceberg connector caches the table's UUID at 
query-plan time. The CTAS transaction flow is:
   beginCreateTable → beginInsert → commitTransaction.
   When replace=true, we dropped and recreated the table inside 
beginCreateTable. But beginInsert then calls beginTransaction → refresh(), 
which compares the cached UUID against the new table and throws 
IllegalStateException: Table UUID does not match: current=<old> != 
refreshed=<new>. There is no public API in the internal connector to reset this 
cache.
   
   Throwing NOT_SUPPORTED is the standard Trino pattern for operations the 
connector genuinely cannot execute, it's what the default ConnectorMetadata 
base class does for any unimplemented method. The error message surfaces 
cleanly to the user: "This connector does not support replacing a table".
   
   I'm happy to add an inline comment in the code explaining the UUID cache 
incompatibility so future maintainers understand why replace=true is rejected 
rather than implemented. I can also add a note to the docs if there's a 
connector-limitations section to update, please point me to the right file. 
Would that address the concern?
   
   



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to