Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/23113 )
Change subject: IMPALA-13267: Display number of partitions for Iceberg tables (WIP) ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/23113/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23113/4//COMMIT_MSG@9 PS4, Line 9: Query plans display the number of scanned partitions correctly for It would be good to mention what the behaviour before this change is/was, i.e. that only a single partition is reported. http://gerrit.cloudera.org:8080/#/c/23113/4/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/23113/4/common/thrift/CatalogObjects.thrift@642 PS4, Line 642: optional Does this struct make sense with this fields being null? http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java: http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@56 PS4, Line 56: * Should be immutable because it is shared between queries on the Coordinator side. Could you make it more precise? This class contains ConcurrentHashMaps which would not be needed if it was immutable. If only a set of its fields should be immutably, could you list them, and also when (after what initialisation) they become immutable? http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@158 PS4, Line 158: Integer Could you include in a comment what this integer stands for? http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@245 PS4, Line 245: //if (!oldPartitionMap_.containsKey(partition)) return null; Dead code. http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2108 PS4, Line 2108: output.append(getPartitionExplainString(detailPrefix,table,testTableSize)); Nit: add spaces after the commas. http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2192 PS4, Line 2192: output By taking the StringBuilder as a parameter, we could save a copy. http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2240 PS4, Line 2240: Long Is there a reason why this function cannot take 'long'? I think that would be clearer. http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@61 PS4, Line 61: Integer Could you add what this integer refers to? http://gerrit.cloudera.org:8080/#/c/23113/4/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@61 PS4, Line 61: Map We only use the number of partitions in this class, so it would be enough to store that. http://gerrit.cloudera.org:8080/#/c/23113/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test: http://gerrit.cloudera.org:8080/#/c/23113/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-merge-insert-only.test@8 PS4, Line 8: 130MB In many cases, memory estimates and row sizes are significantly higher. What could cause this? Did the table definitions change? -- To view, visit http://gerrit.cloudera.org:8080/23113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb2f654bc6c9bdf9cfafc27b38b5ca2f7b6b4872 Gerrit-Change-Number: 23113 Gerrit-PatchSet: 4 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 25 Jul 2025 17:31:12 +0000 Gerrit-HasComments: Yes
