Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17857 )
Change subject: IMPALA-10914: Consistently schedule scan ranges for Iceberg tables ...................................................................... Patch Set 1: (8 comments) Looks very reasonable. It is not clear to me how a user select a particular iceberg table snapshot to use in a query. http://gerrit.cloudera.org:8080/#/c/17857/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17857/1//COMMIT_MSG@11 PS1, Line 11: and the HDFS : block locations were not consistent across the reload > This is not clear to me: if the block locations have changed between querie +1. I think the fix is applicable when an iceberg table of a particular snapshot is to be scanned. In doing so for the same table instance, the partition info about the snapshot can be reused. When only the last version of the table is to be scanned, my impression is that the patch should not be applied. http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java@110 PS1, Line 110: "Failed to load table: %s.%s", msTable.getDbName(), msTable.getTableName()), e); nit probably should include the snapshot id in the message, if it is not in e. http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1227 PS1, Line 1227: a nit. an http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1268 PS1, Line 1268: addSummary(response, "Updated table."); Updated table after set table properties http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1272 PS1, Line 1272: Updated table. Updated table after unset table properties http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1295 PS1, Line 1295: if (!needsToUpdateHms) { : loadTableMetadata(tbl, newCatalogVersion, true, true, null, "ALTER Iceberg TABLE " + : params.getAlter_type().name()); : catalog_.addVersionsForInflightEvents(false, tbl, newCatalogVersion); : addTableToCatalogUpdate(tbl, wantMinimalResult, response.result); : return false; : } Should this block of code be executed within 1283-1286 too? That is, if txn is needed, then this block of code is protected by the txn. http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@113 PS1, Line 113: addColumn nit. addColumns()? http://gerrit.cloudera.org:8080/#/c/17857/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@339 PS1, Line 339: the table parameters nit. update property in 'txn' -- To view, visit http://gerrit.cloudera.org:8080/17857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb8216b37d350469b573dad7fcefdd0ee0599ed5 Gerrit-Change-Number: 17857 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 21 Sep 2021 17:23:08 +0000 Gerrit-HasComments: Yes
