Arnab Karmakar 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 4: (7 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: // ONLY supported for HDFS tables (FeFsTable). > nit: called whereClause_ in SelectStmt. And you call it a "WHERE clause" in Done http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@206 PS3, Line 206: if (!compoundVerticalBarExprs.isEmpty()) { > line too long (91 > 90) Done 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: totalNumRows += stats[3]; > nit: I think it is better to unify getTableStats() and getTableStatsForPart Done http://gerrit.cloudera.org:8080/#/c/23566/3/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@618 PS3, Line 618: totalNumRows += stats[3]; > I agree, it seems to be exactly the same except you need to deal with 3 sta Done 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 ((FeFsTable) table).getTableStats(); > Not directly related to this patch, but should this be an instance of FeFsT Not sure if I got your question properly, but we are handling each kind of table instance with different ifs right? FeFsTable is being taken care of above. 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.getTable_name().getTable_name(), params.op, > Please fix this. Done 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: // WHERE clause with Kudu table should fail (non-HDFS tables don't support WHERE) > I'm not sure a Kudu table actually works. Please test it; might as well add Yes you are right, thanks for pointing it out. Im removing the hbase test case(tc) because: 1. We've already got one hbase tc above 2. HBase tables don't support SHOW PARTITIONS at all (with or without WHERE), so we get the more general error message saying "SHOW PARTITIONS must target an HDFS or Kudu table". Adding a new Kudu tc that covers the "WHERE clause not supported on non-HDFS tables" scenario. -- 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: 4 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: Fri, 14 Nov 2025 10:07:14 +0000 Gerrit-HasComments: Yes
