Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16266 )
Change subject: IMPALA-5022: Outer join simplification ...................................................................... Patch Set 10: (11 comments) Thanks a lot for the code change. I do have some comments. 1). The UDF exclusion has not been added yet; 2). Some conversions seem not correct to me, due to the missing null-reject predicates on the join columns. http://gerrit.cloudera.org:8080/#/c/16266/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16266/10//COMMIT_MSG@9 PS10, Line 9: e nit. It may be useful to break long text lines so that each is of 90 characters or less. http://gerrit.cloudera.org:8080/#/c/16266/10//COMMIT_MSG@22 PS10, Line 22: 2. nit. It may be nice to insert a new line after each example to allow better readability. http://gerrit.cloudera.org:8080/#/c/16266/10//COMMIT_MSG@27 PS10, Line 27: : 5. These are very good examples. Can you add one more for the following? A full join B on A.id = B.id where A.u > 10 and B.v > 10 http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/16266/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3289 PS5, Line 3289: > We need skip this, maybe have other null-rejecting conjuncts Done http://gerrit.cloudera.org:8080/#/c/16266/10/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/16266/10/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@3348 PS10, Line 3348: a nit. an outer join http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/card-outer-join.test File testdata/workloads/functional-planner/queries/PlannerTest/card-outer-join.test: http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/card-outer-join.test@95 PS10, Line 95: t outer join tpch.orders o on c.c_custkey = o.o_custkey : where c.c_name = 'foo' Seems to me the conversion is not correct (no null-rejecting predicate on the join column exists). http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test File testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test: http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@25 PS10, Line 25: # Tests where clause containing disjunctive conjuncts I like the comment above about the expectation of the result "so we can't convert a left join to an inner join". The comment is important to verify the result now and in the future. Can we add the expectation comment for other tests in this file? http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@52 PS10, Line 52: on t1.id = t2.id : where t1.int_col + t2.int_col < 10 or t2.tinyint_col < 5 or t2.smallint_col > 2 Converting it to inner join seems a bug to me, as I did not see a null-rejecting predicate on t2.id in the WHERE clause. http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@242 PS10, Line 242: by ZEROIFNULL(t2.zip) < t3.test_zip The ZEROIFNULL test on t2.zip does not provide information on whether t2.id is null or not. Not sure if this query can be converted. http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@293 PS10, Line 293: INNER JOIN same comment as above. http://gerrit.cloudera.org:8080/#/c/16266/10/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@312 PS10, Line 312: We can't simplify outer join executing after inner join by inner join on clause This is a very good test case. -- To view, visit http://gerrit.cloudera.org:8080/16266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa7804033fac68e93f33c387dc68ef67f803e93e Gerrit-Change-Number: 16266 Gerrit-PatchSet: 10 Gerrit-Owner: Xianqing He <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Mon, 24 Aug 2020 20:05:39 +0000 Gerrit-HasComments: Yes
