Tim Armstrong 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 9: (7 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. Mention how 'candidates' is used? http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@61 PS9, Line 61: (E nit: these parens prob aren't needed, right? http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@66 PS9, Line 66: !(bp.getOp() == BinaryPredicate.Operator.EQ) can't this be !=? http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@128 PS9, Line 128: Map.Entry Can't this be Map.Entry<Integer, Expr> to keep it type-safe and avoid cast? http://gerrit.cloudera.org:8080/#/c/16346/9/fe/src/main/java/org/apache/impala/analysis/ConstantPredicateHandler.java@132 PS9, Line 132: Map.Entry Map.Entry<SlotRef, List<Expr>>? 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' Don't we still need to keep the date_col = cast(timestamp_col as date) predicate for this to be correct in cases where this isn't guaranteed to be true in the underlying data set? E.g. one counter-example would be date_col timestamp_col 2009-12-01 2009-12-2 00:00:00 I.e. I think we need to keep the equality predicate around. http://gerrit.cloudera.org:8080/#/c/16346/9/testdata/workloads/functional-query/queries/QueryTest/range-constant-propagation.test File testdata/workloads/functional-query/queries/QueryTest/range-constant-propagation.test: http://gerrit.cloudera.org:8080/#/c/16346/9/testdata/workloads/functional-query/queries/QueryTest/range-constant-propagation.test@4 PS9, Line 4: functional_parquet We generally don't include database names in the test files, since the infra should switch to the appropriate functional database. We can move it to TestQueriesParquetTables if we only want it to run on the parquet data set (not kudu, etc). -- 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: 9 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 01:17:03 +0000 Gerrit-HasComments: Yes
