Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/23566 )
Change subject: IMPALA-14065: Support WHERE clause in SHOW PARTITIONS statement ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/23566/3/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/23566/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@56 PS3, Line 56: protected Expr whereExpr_; nit: called whereClause_ in SelectStmt. And you call it a "WHERE clause" in a comment later on. http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@206 PS3, Line 206: // Expression needs rewriting - defer partition filtering to second analysis pass > line too long (91 > 90) Please address this comment. http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java File fe/src/main/java/org/apache/impala/catalog/FeFsTable.java: http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@618 PS3, Line 618: default TResultSet getTableStatsForPartitions(Collection<Long> partitionIds) { > nit: I think it is better to unify getTableStats() and getTableStatsForPart I agree, it seems to be exactly the same except you need to deal with 3 states: 1. load all partitions 2. load specified partitions from partitionIds 3. load no partitions because partitionIds is empty Handling that with partitionIds is null, not empty, empty seems fine. I don't mind some of the other refactoring to create smaller methods though. http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/service/Frontend.java@1766 PS3, Line 1766: return ((FePaimonTable) table).getTableStats(op); Not directly related to this patch, but should this be an instance of FeFsTable? http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java@463 PS3, Line 463: params.isSetFiltered_partition_ids() ? params.getFiltered_partition_ids() : null); > line too long (92 > 90) Please fix this. http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4481 PS3, Line 4481: "SHOW PARTITIONS must target an HDFS or Kudu table: functional_hbase.alltypes"); I'm not sure a Kudu table actually works. Please test it; might as well add a case here with functional_kudu. -- To view, visit http://gerrit.cloudera.org:8080/23566 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e2a14aabcea3fb17083d4ad6f87b7861113f89e Gerrit-Change-Number: 23566 Gerrit-PatchSet: 3 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 12 Nov 2025 22:29:02 +0000 Gerrit-HasComments: Yes
