Baike Xia has posted comments on this change. ( http://gerrit.cloudera.org:8080/18731 )
Change subject: IMPALA-11424: Support pushdown non-equi join predicate from OUTER/INNER JOIN to SCANNODE ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/18731/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18731/11//COMMIT_MSG@13 PS11, Line 13: 1. Only support LEFT_OUTER_JOIN, RIGHT_OUTER_JOIN, INNER_JOIN; > Is there a specific reason for not supporting cross joins? cross join was rarely used, and the optimization was not verified. Spark does not support cross join by default, and Presto/Trino optimizes to eliminate cross join. So We are not going to consider cross join. http://gerrit.cloudera.org:8080/#/c/18731/11/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18731/11/common/thrift/Query.thrift@600 PS11, Line 600: // Whether to enable pushdown under non-equi predicates, default is false > missing line Got. http://gerrit.cloudera.org:8080/#/c/18731/11/common/thrift/Query.thrift@600 PS11, Line 600: // Whether to enable pushdown under non-equi predicates, default is false : 149: optional bool enable_none_equal_predicate_push_down = false; > What is the reason for keeping it as default false? My understanding is tha This change will change the execution plan, and I am worried that it may affect the results. Moreover, the default value is true, which will lead to the rewriting of many existing ut test data, which may introduce other problems. Therefore, I want to default to false first, and then change it to true after a period of time http://gerrit.cloudera.org:8080/#/c/18731/11/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/18731/11/fe/src/main/java/org/apache/impala/analysis/Expr.java@1933 PS11, Line 1933: weight > What does removing weight means here? This is a misstatement. What I want to say is, remove duplicates I will fix it. http://gerrit.cloudera.org:8080/#/c/18731/9/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: http://gerrit.cloudera.org:8080/#/c/18731/9/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@791 PS9, Line 791: binaryPredicate = new BinaryPredicate(GE, slotBinding, minValue); > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/18731/11/testdata/workloads/functional-planner/queries/PlannerTest/none-equal-predicate-push-down.test File testdata/workloads/functional-planner/queries/PlannerTest/none-equal-predicate-push-down.test: http://gerrit.cloudera.org:8080/#/c/18731/11/testdata/workloads/functional-planner/queries/PlannerTest/none-equal-predicate-push-down.test@5 PS11, Line 5: testtbl > I think that it would be better to use a non-empty table like functional.al Yes, that's right. I will add relevant tests. Thanks. http://gerrit.cloudera.org:8080/#/c/18731/11/testdata/workloads/functional-planner/queries/PlannerTest/none-equal-predicate-push-down.test@7 PS11, Line 7: 2 > I only see numbers in the tests - does this optimization work with other ty This optimization is valid for expr in LiteralExpr. I didn't add another type of test. I will add relevant tests. Thanks. http://gerrit.cloudera.org:8080/#/c/18731/9/tests/query_test/test_none_equi_predicate_pushdown.py File tests/query_test/test_none_equi_predicate_pushdown.py: http://gerrit.cloudera.org:8080/#/c/18731/9/tests/query_test/test_none_equi_predicate_pushdown.py@23 PS9, Line 23: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/18731/11/tests/query_test/test_none_equi_predicate_pushdown.py File tests/query_test/test_none_equi_predicate_pushdown.py: http://gerrit.cloudera.org:8080/#/c/18731/11/tests/query_test/test_none_equi_predicate_pushdown.py@32 PS11, Line 32: ENABLE_NONE_EQUAL_PREDICATE_PUSH_DOWN > Can you also run to same test case with ENABLE_NONE_EQUAL_PREDICATE_PUSH_DO OK, I will add it. -- To view, visit http://gerrit.cloudera.org:8080/18731 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3ce23cbd7522a209c830504f329b972d67bc263 Gerrit-Change-Number: 18731 Gerrit-PatchSet: 11 Gerrit-Owner: Baike Xia <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Baike Xia <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 25 Oct 2022 04:06:21 +0000 Gerrit-HasComments: Yes
