Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16346 )
Change subject: IMPALA-10064: Support constant propagation for eligible range predicates ...................................................................... Patch Set 8: (7 comments) 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: 10064 > Wrong JIRA? Oops ! Changed it to 10064. 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: ) { > Are these constructor arguments required? Since you keep some state in this Right..not needed. I had a version earlier using the arguments then forgot to remove them. I have removed them. The second part of your comment wasn't quite clear. I did not want to maintain more state than necessary in the class. Also note below that the first method's bitset is the input candidate bitset while the second method's bitset is the output 'changed' bitset. http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@65 PS7, Line 65: BinaryPredicate.IS_RANGE_PREDICATE.apply(bp); : if (!isRangeOp && !(bp.getOp() == BinaryPredicate.Operator.EQ)) continue; : SlotRef slotRef = bp.getBoundSlot(); : if (slotRef == null || !bp.getChild(1).isConsta > *Suggestion* If you want to make this check a static public method in Bina Makes sense to create a static api for this. I followed the com.google.common.base.Predicate<T>.apply() convention that is used in the Expr class but since the expr is already downcast to BinaryPredicate, I created the the api in the BinaryPredicate. Also modified HdfsScanNode accordingly. 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. Done http://gerrit.cloudera.org:8080/#/c/16346/7/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@180 PS7, Line 180: : : > Not sure that this is a very useful exception. You could do: Done 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 ma Hmm..when I restricted it to text/none, the load script would do 2 loads..one through Impala and again through a Hive command . The latter generated this: 'Beginning execution of hive SQL' message which would fail due to something related to static vs dynamic partitioning. Is there a way to exclude Hive from the load process ? With parquet format I did not encounter this. 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> <s It should work but will add a test for it. -- 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: 8 Gerrit-Owner: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 28 Aug 2020 01:37:50 +0000 Gerrit-HasComments: Yes
