[Impala-ASF-CR] IMPALA-11008: fix incorrect to propagate inferred predicates

["Qifan Chen (Code Review)" <ge...@cloudera.org>]

Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/18234 )

Change subject: IMPALA-11008: fix incorrect to propagate inferred predicates
......................................................................


Patch Set 7:

(13 comments)

This is a great fix.

http://gerrit.cloudera.org:8080/#/c/18234/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18234/7//COMMIT_MSG@11
PS7, Line 11: condition
nit. may remove.


http://gerrit.cloudera.org:8080/#/c/18234/7//COMMIT_MSG@27
PS7, Line 27: Because
nit. This is incorrect since


http://gerrit.cloudera.org:8080/#/c/18234/7//COMMIT_MSG@29
PS7, Line 29: condition
nit. remove


http://gerrit.cloudera.org:8080/#/c/18234/7/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/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@513
PS7, Line 513: get
nit. delete


http://gerrit.cloudera.org:8080/#/c/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@514
PS7, Line 514: in
nit. be added to


http://gerrit.cloudera.org:8080/#/c/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@515
PS7, Line 515: ojNullableEqualSlots
nit. maybe ojNullableSlotsInEquiPreds?


http://gerrit.cloudera.org:8080/#/c/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2124
PS7, Line 2124: conditions for the nullable side
nit. remove


http://gerrit.cloudera.org:8080/#/c/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2130
PS7, Line 2130:           // We use 'ojmap' to save src to outer join equal slot, eg. B.id. We substitue
nit. save the mapping from src to the outer join equal slot.


http://gerrit.cloudera.org:8080/#/c/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2150
PS7, Line 2150: if (ojSmap.size() != 0) {
              :               p = p.trySubstitute(ojSmap, this, false);
              :             }
I wonder if this is a redundant operation since the content of ojmap is a subset of that of smap.


http://gerrit.cloudera.org:8080/#/c/18234/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2729
PS7, Line 2729:  (tblRef.getJoinOp() == JoinOperator.LEFT_OUTER_JOIN ||
              :           tblRef.getJoinOp() == JoinOperator.RIGHT_OUTER_JOIN
Should we handle FULL_OUTER_JOIN and ANTI joins?


http://gerrit.cloudera.org:8080/#/c/18234/7/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
File testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test:

http://gerrit.cloudera.org:8080/#/c/18234/7/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1593
PS7, Line 1593: # IMPALA-11008: fix incorrect to propagate inferred predicates
nit. may add a comment here to state the potential push-down predicate filters on NULL and is thus not push-able.

The same argument applies to other new negative tests in this file.


http://gerrit.cloudera.org:8080/#/c/18234/7/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1603
PS7, Line 1603: LEFT JOIN
We may need to add a couple of RIGHT JOIN queries.


http://gerrit.cloudera.org:8080/#/c/18234/7/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1723
PS7, Line 1723: t2.id + t2.zip = 2
This is cool.



-- 
To view, visit http://gerrit.cloudera.org:8080/18234
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e64230f6d0c2b9ef1560186ceba349a5920ccdf
Gerrit-Change-Number: 18234
Gerrit-PatchSet: 7
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Tue, 22 Mar 2022 16:26:05 +0000
Gerrit-HasComments: Yes