Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24049 )
Change subject: WIP IMPALA-14805: LocalIcebergTable loads files in coordinator ...................................................................... Patch Set 15: (5 comments) Nice patch! Only found minor issues. http://gerrit.cloudera.org:8080/#/c/24049/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24049/15//COMMIT_MSG@19 PS15, Line 19: load_iceberg_files_in_coordinator=true does not : work with legacy catalog mode We could check incompatible flag combinations at startup. http://gerrit.cloudera.org:8080/#/c/24049/15/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/24049/15/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1354 PS15, Line 1354: // TODO: rethink how to get api table : TableParams tableParams = new TableParams(tableRef.msTable_); : org.apache.iceberg.Table apiTable = : loadIcebergApiTable(tableRef, tableParams, tableRef.msTable_); : : // Try to lookup the last stored IcebergContentFileStore to help with : // incremental loading. A weak reference is used to allow it to be GCd : // if it gets evicted from cache. The key doesn't include catalog version, : // so there is always only one lastFileStore per table. : // The weak reference and key have non-zero weight, so the entry can be evicted : // by weight limit. The weigher does not go through weak references, so the : // backing file store won't be double counted. : LastIcebergContentFileStoreCacheKey tableKey = : new LastIcebergContentFileStoreCacheKey(tableRef); : WeakReference<MetaProvider.CachedIcebergFiles> lastFileStore = : (WeakReference<MetaProvider.CachedIcebergFiles>) : cache_.getIfPresent(tableKey); This is not needed when the files are in the cache. Would it be worth adding a quick getIfPresent(cacheKey) check? Or maybe they could be moved inside call() via some extra indirection. http://gerrit.cloudera.org:8080/#/c/24049/15/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/24049/15/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@599 PS15, Line 599: o nit: missing space http://gerrit.cloudera.org:8080/#/c/24049/15/fe/src/main/java/org/apache/impala/service/BackendConfig.java File fe/src/main/java/org/apache/impala/service/BackendConfig.java: http://gerrit.cloudera.org:8080/#/c/24049/15/fe/src/main/java/org/apache/impala/service/BackendConfig.java@660 PS15, Line 660: Incoordinator nit: InCoordinator http://gerrit.cloudera.org:8080/#/c/24049/15/tests/custom_cluster/test_iceberg_load_on_coordinator.py File tests/custom_cluster/test_iceberg_load_on_coordinator.py: http://gerrit.cloudera.org:8080/#/c/24049/15/tests/custom_cluster/test_iceberg_load_on_coordinator.py@32 PS15, Line 32: # TODO: think through what tests to run in with special config. How much time does it take to run these tests? Since the tests are running sequentially, maybe we should only run them in exhaustive mode, or merge a few together? -- To view, visit http://gerrit.cloudera.org:8080/24049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6732af76a2e040fa57e39260302951466037b934 Gerrit-Change-Number: 24049 Gerrit-PatchSet: 15 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 29 Jun 2026 15:39:00 +0000 Gerrit-HasComments: Yes
