Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/23800 )
Change subject: IMPALA-14555: Add Iceberg support for SHOW PARTITIONS WHERE ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/23800/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23800/2//COMMIT_MSG@33 PS2, Line 33: Some code parts look AI-generated. Please include 'Generated-by:' tag: https://www.apache.org/legal/generative-tooling.html http://gerrit.cloudera.org:8080/#/c/23800/2/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/23800/2/common/thrift/Frontend.thrift@257 PS2, Line 257: // Only used for HDFS tables (FeFsTable, excluding FeIcebergTable). nit: this comment is a bit odd, maybe just "Only used for HDFS tables"? http://gerrit.cloudera.org:8080/#/c/23800/2/common/thrift/Frontend.thrift@260 PS2, Line 260: iceberg nit: Iceberg http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionStatsHelper.java File fe/src/main/java/org/apache/impala/analysis/IcebergPartitionStatsHelper.java: http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionStatsHelper.java@32 PS2, Line 32: public class IcebergPartitionStatsHelper { I think this class is not really needed; the computeFilteredPartitionStats function could be added to ShowStatsStmt. http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionStatsHelper.java@72 PS2, Line 72: Expr rewrittenExpr = BetweenToCompoundRule.INSTANCE.apply(whereClause, analyzer); Please add BETWEEN rewriting to IcebergPartitionExpressionRewriter instead http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionStatsHelper.java@97 PS2, Line 97: if (foldedExpr instanceof BoolLiteral) { : BoolLiteral boolLiteral = (BoolLiteral) foldedExpr; : if (boolLiteral.getValue()) { : // Folded to TRUE - return all partitions (no filter) : return FeIcebergTable.Utils.getPartitionStats(table); : } else { : // Folded to FALSE - return empty result set : return FeIcebergTable.Utils.createEmptyPartitionStatsResult(); : } : } This check should be pushed down to getPartitionStats instead, where the Iceberg expression is already there. I think there's an option to check if the Iceberg expression is simply just true or false. The same goes for L58 http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionStatsHelper.java@108 PS2, Line 108: // Use Iceberg's efficient filtering API for simple predicates This comment seems a bit odd; there's no filtering API called here, the predicate converter just translates the Impala expression tree to an Iceberg expression tree. http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java: http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@55 PS2, Line 55: // Supported for both HDFS tables (FeFsTable) and Iceberg tables (FeIcebergTable). nit: it sounds a bit artificial, consider: "Supported for HDFS and Iceberg tables" http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@525 PS2, Line 525: fe iceberg table nit: Iceberg table http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@598 PS2, Line 598: org.apache.iceberg.TableScan scan = icebergTable.newScan().filter(filterExpr); Please use IcebergUtil.planFiles http://gerrit.cloudera.org:8080/#/c/23800/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@1039 PS2, Line 1039: public static String getPartitionKey(IcebergTable table, ContentFile<?> contentFile) { Is this required? getPartitionKey is only used inside this interface, you can safely migrate it to FeIcebergTable -- To view, visit http://gerrit.cloudera.org:8080/23800 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c0ee4d171ae939770725d89dc504e13f82a7688 Gerrit-Change-Number: 23800 Gerrit-PatchSet: 2 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 07 Jan 2026 13:39:33 +0000 Gerrit-HasComments: Yes
