Shant Hovsepian has posted comments on this change. ( http://gerrit.cloudera.org:8080/16346 )
Change subject: IMPALA-10046: Support constant propagation for eligible range predicates ...................................................................... Patch Set 7: (7 comments) Nice addition. http://gerrit.cloudera.org:8080/#/c/16346/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16346/7//COMMIT_MSG@7 PS7, Line 7: 10046 Wrong JIRA? http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java File fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java: http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@45 PS7, Line 45: List<Expr> conjuncts, BitSet candidates Are these constructor arguments required? Since you keep some state in this class it might be cleaner to initialize an instance just for specific arguments here and then now require arguments for the methods below. http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@65 PS7, Line 65: (bp.getOp() == BinaryPredicate.Operator.LE || : bp.getOp() == BinaryPredicate.Operator.LT || : bp.getOp() == BinaryPredicate.Operator.GE || : bp.getOp() == BinaryPredicate.Operator.GT); *Suggestion* If you want to make this check a static public method in BinaryPredicate it would be nice. You could clean up some code in HdfsScanNode that is similar for MinMax Predicates. http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@164 PS7, Line 164: This can be static if you choose. http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@180 PS7, Line 180: } catch (Exception e) { : throw new IllegalStateException("Failed analysis on new predicate", e); : } Not sure that this is a very useful exception. You could do: newPred.analyzeNoThrow(analyzer) http://gerrit.cloudera.org:8080/#/c/16346/7/testdata/datasets/functional/schema_constraints.csv File testdata/datasets/functional/schema_constraints.csv: http://gerrit.cloudera.org:8080/#/c/16346/7/testdata/datasets/functional/schema_constraints.csv@22 PS7, Line 22: table_format:parquet Usually these small test table are text types, any particular reason for making it parquet? http://gerrit.cloudera.org:8080/#/c/16346/7/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test File testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test: http://gerrit.cloudera.org:8080/#/c/16346/7/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test@461 PS7, Line 461: timestamp_col <= '2010-12-01'; There is a NormalizeBinaryPredicate rule that should rewrite "const <op> <slotref>" into "<slotref> <op> const" but I couldn't tell if it runs before or after propagateConstants() might be good to test that case as well. -- To view, visit http://gerrit.cloudera.org:8080/16346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I811a1f8d605c27c7704d7fc759a91510c6db3c2b Gerrit-Change-Number: 16346 Gerrit-PatchSet: 7 Gerrit-Owner: Aman Sinha <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 27 Aug 2020 23:35:51 +0000 Gerrit-HasComments: Yes
