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 10:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/16346/9/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/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@55
PS9, Line 55:    * predicates. The candidates BitSet is used to determine which 
members of
> Mention how 'candidates' is used?
Done


http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@61
PS9, Line 61: co
> nit: these parens prob aren't needed, right?
Done


http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@66
PS9, Line 66:  = BinaryPredicate.IS_RANGE_PREDICATE.apply(b
> can't this be !=?
Done


http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@128
PS9, Line 128: opagation
> Can't this be Map.Entry<Integer, Expr> to keep it type-safe and avoid cast?
Done


http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@132
PS9, Line 132: dtExpr = d
> Map.Entry<SlotRef, List<Expr>>?
Done


http://gerrit.cloudera.org:8080/#/c/16346/9/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/9/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test@419
PS9, Line 419:    predicates: timestamp_col <= TIMESTAMP '2010-12-01 00:00:00', 
timestamp_col >= TIMESTAMP '2009-12-01 00:00:00', date_col = CAST(timestamp_col 
AS DATE)
> Good point.  All the use cases I have seen so far were ones where date_col
Made the code change to preserve the original conjunct  date_col = 
cast(timestamp_col as date).  Updated plans.
TODO:  Add e2e test against a dataset where date values != timestamp col's date 
component.



--
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: 10
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: Tue, 01 Sep 2020 06:59:28 +0000
Gerrit-HasComments: Yes

Reply via email to