Peter Rozsa has posted comments on this change. (
http://gerrit.cloudera.org:8080/24682 )
Change subject: IMPALA-15310: Finalize REST catalog INSERT
......................................................................
Patch Set 11:
(3 comments)
In most places this is a bolted-on solution rather than a generalized
one. The following are hooks grafted onto the existing Iceberg/REST-catalog
flow,
not general abstractions:
- the opaque finalizer id (really just the REST catalog name)
- the reused TUpdateCatalogRequest routed by field presence
- the provider-derived capability flag
- the per-statement DML gating (OperationType.INSERT_INTO +
hand-wired ensureNonInsertDmlSupported calls)
http://gerrit.cloudera.org:8080/#/c/24682/11/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:
http://gerrit.cloudera.org:8080/#/c/24682/11/common/thrift/CatalogService.thrift@314
PS11, Line 314: // Identifies the metadata provider that owns
coordinator-side Iceberg DML
This is a semantic overload: one struct now means two different destinations
depending on whether one optional field is set. The routing decision lives in
client-request-state.cc as an if (__isset.iceberg_dml_finalizer_id). It works,
but it's a bolted-on branch on a shared message. A cleaner model would be a
distinct request type (or a required destination enum) so the routing is
explicit in the type rather than inferred from field presence. Given the
coordinator already fully populates the struct, this may be acceptable, but
it's the kind of thing that accumulates.
http://gerrit.cloudera.org:8080/#/c/24682/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/24682/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@322
PS11, Line 322: INSERT_INTO,
OperationType is a table-access-capability axis: READ, WRITE, ANY. The commit
added INSERT_INTO, which is a SQL-statement distinction, not a capability
distinction. ensureTableWriteSupported should be extended instead.
http://gerrit.cloudera.org:8080/#/c/24682/11/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java
File
java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java:
http://gerrit.cloudera.org:8080/#/c/24682/11/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@148
PS11, Line 148: private void prepareWriteTestTable(Catalog catalog, String
tableName)
The test table creation should live in the test files. The test server is
already a REST-capable server and the python test code is capable of calling
the create table endpoint without any problems. It would keep the test server
clean, and give the capabilities to future tests to create tables in catalogs
--
To view, visit http://gerrit.cloudera.org:8080/24682
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d17216fb8d12e5bd007b93cb9b3bc153438213b
Gerrit-Change-Number: 24682
Gerrit-PatchSet: 11
Gerrit-Owner: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: Aleksandr Efimov <[email protected]>
Gerrit-Reviewer: Daniel Becker <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]>
Gerrit-Reviewer: Peter Rozsa <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Tue, 08 Sep 2026 15:27:00 +0000
Gerrit-HasComments: Yes
