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: (3 comments) http://gerrit.cloudera.org:8080/#/c/16346/8/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/8/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@78 PS8, Line 78: isRangeOp && !(constant instanceof DateLiteral || : constant instanceof TimestampLiteral)) > For extensibility, maybe we could add a new argument to the method to indic It is kind of tricky to make it extensible in that sense because the logic for the range predicate propagation is closely tied with date and timestamp types. It is intentional to keep it limited to these types since semantically it may not be correct for other types. Let me know if you are aware of other products doing the propagation for any other data type. http://gerrit.cloudera.org:8080/#/c/16346/8/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@93 PS8, Line 93: Propagate equality constant predicates to other conjuncts. Propagate : * range constant predicates to conjuncts involving date and timestamp : * columns. > I wonder if the propagation here happens only for the predicates appear in So the join scenario is handled differently. The planner has the notion of a ValueTransferGraph (see [1]) which is a strongly connected graph built using equijoin predicates. This allows transitive closure of the predicates and constant predicates are transitively applied to the other side of the join. In your example, yeah this will create a new predicate s.b = 2. The constant predicate propagation comes into the picture only when we build the Scan operator (after the above phase is done), so at this point only single table predicates are passed into the Expr.optimizeConjuncts(List<Expr> conjuncts, Analyzer analyzer) method. [1] https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/Analyzer.java#L2358 http://gerrit.cloudera.org:8080/#/c/16346/8/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/8/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test@410 PS8, Line 410: select * from functional_parquet.alltypes_date_partition : where date_col = cast(timestamp_col as date) : and timestamp_col between '2009-12-01' and '2010-12-01'; > Can we add one more test here? Actually were were never supporting constant propagation where the source is a disjunctive predicate. Even for equality conditions, this is the plan: explain select * from functional_parquet.alltypes_date_partition where int_col = cast(bigint_col as int) and (bigint_col = 50 or bigint_col = 100) | | | 00:SCAN HDFS [functional_parquet.alltypes_date_partition] | HDFS partitions=730/730 files=730 size=2.22MB | predicates: int_col = CAST(bigint_col AS INT), bigint_col IN (50, 100) | row-size=65B cardinality=326 Currently, we only allow BinaryPredicate to be the one that triggers the propagation, not CompoundPredicate. I suppose for simple CompoundPredicates such the one above where both children are BinaryPredicate, one could propagate but in the general case it would be complicated. -- 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 <[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: Fri, 28 Aug 2020 17:34:51 +0000 Gerrit-HasComments: Yes
