Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/25039 )
Change subject: IMPALA-15493: Coordinator-local DDL execution for Iceberg REST catalogs ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@123 PS3, Line 123: if (answer.first) return answer.second; What if the database exists in more than one catalog? When the table fails to load, this picks the first catalog with that database, which may not own the table. Could we use the table name to find the right catalog without loading its metadata? http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@125 PS3, Line 125: return null; With multiple REST catalogs and no CatalogD, DROP TABLE IF EXISTS missing_db.t ends up failing with “Operation is not supported without CatalogD.” The same query works with one REST catalog. Could we make it a no-op in both cases and add a test? http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/service/Frontend.java@699 PS3, Line 699: return getCatalogNameForDdl(catalogManager_, stmt.getTable(), stmt.getDb()); I think we still miss the single-provider case here. If metadata loading throws a RESTException, getTableIfPresent() lets it through and StmtMetadataLoader fails before DROP analysis. Could we add a test with unreadable metadata and one REST catalog? http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/service/IcebergDdlExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergDdlExecutor.java: http://gerrit.cloudera.org:8080/#/c/25039/3/fe/src/main/java/org/apache/impala/service/IcebergDdlExecutor.java@115 PS3, Line 115: .dropTable(dbName, tblName, params.purge); Looks like this bypasses the blacklist check. A blacklisted REST table is hidden during analysis, but DROP TABLE IF EXISTS still gets here and deletes it. With PURGE, it also deletes the data. Could we keep the CatalogD behavior and add a test? -- To view, visit http://gerrit.cloudera.org:8080/25039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I932a211b25ea4e24a607d047c35da186d8922d02 Gerrit-Change-Number: 25039 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Wed, 07 Oct 2026 12:03:40 +0000 Gerrit-HasComments: Yes
