Quanlong Huang 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 10: (9 comments) Thanks to refactor the patch. It looks much better now. http://gerrit.cloudera.org:8080/#/c/23566/10/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/10/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@206 PS10, Line 206: List<CompoundVerticalBarExpr> compoundVerticalBarExprs = new ArrayList<>(); : whereClause_.collectAll(Predicates.instanceOf(CompoundVerticalBarExpr.class), : compoundVerticalBarExprs); : if (!compoundVerticalBarExprs.isEmpty()) { : // Expression needs rewriting - defer partition filtering to second analysis. : return; : } Please add tests for such expressions. http://gerrit.cloudera.org:8080/#/c/23566/10/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@224 PS10, Line 224: for (Expr conjunct : whereClause_.getConjuncts()) { : if (!conjunct.isBoundBySlotIds(partitionSlots)) { : throw new AnalysisException( : "SHOW PARTITIONS WHERE supports only partition columns"); : } Can we simply check whereClause_.isBoundBySlotIds(partitionSlots) ? http://gerrit.cloudera.org:8080/#/c/23566/10/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@230 PS10, Line 230: if (conjunct.contains(Expr.IS_AGGREGATE)) { : throw new AnalysisException( : "Aggregate functions are not supported in SHOW PARTITIONS WHERE clause"); : } Can we simply check whereClause_.contains(Expr.IS_AGGREGATE) like above for Subquery and AnalyticExpr? http://gerrit.cloudera.org:8080/#/c/23566/10/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@244 PS10, Line 244: pruner.prunePartitions(analyzer, conjunctsCopy, true, tableRef, true); Please add a Precondition check on conjunctsCopy.isEmpty() after this, which means all conjuncts have been evaluated. Preconditions.checkState(conjunctsCopy.isEmpty(), "All conjuncts should be evaluated"); http://gerrit.cloudera.org:8080/#/c/23566/10/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java: http://gerrit.cloudera.org:8080/#/c/23566/10/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java@113 PS10, Line 113: * If 'evalAllFuncs' is True, non-deterministic functions will be evaluated : * per-partition instead of being skipped. nit: please also mention that callers should make sure the conjects are all evaluable in BE to avoid crash. http://gerrit.cloudera.org:8080/#/c/23566/10/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java@118 PS10, Line 118: , boolean evalAllFuncs nit: though we prefer adding new parameters at the end, it looks better to add this after 'allowEmpty' so two boolean parameters are put together. http://gerrit.cloudera.org:8080/#/c/23566/10/testdata/workloads/functional-query/queries/QueryTest/show-stats.test File testdata/workloads/functional-query/queries/QueryTest/show-stats.test: http://gerrit.cloudera.org:8080/#/c/23566/10/testdata/workloads/functional-query/queries/QueryTest/show-stats.test@312 PS10, Line 312: year = 2009 or year = 2010 nit: use 'month' to reduce the result size instead of returning all partitions. E.g. month = 1 or month = 6. http://gerrit.cloudera.org:8080/#/c/23566/10/testdata/workloads/functional-query/queries/QueryTest/show-stats.test@447 PS10, Line 447: # Results are deterministic as rand() without seed produces same value = 0.47... : show partitions alltypes where rand() < 0.5 This is an implementation-specific detail and should not be relied upon. I think running this statement in python code and verifying the statement succeeds is enough. http://gerrit.cloudera.org:8080/#/c/23566/10/testdata/workloads/functional-query/queries/QueryTest/show-stats.test@495 PS10, Line 495: show partitions alltypes where month = month(now()) It would be better to test this in python code where we can verify the results precisely. E.g. https://github.com/apache/impala/blob/321429eac6400fd8c1b22c8aabb2a11fee381437/tests/query_test/test_insert.py#L314-L319 -- 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: 10 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: Peter Rozsa <[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: Tue, 02 Dec 2025 13:30:48 +0000 Gerrit-HasComments: Yes
