You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Xianqing He (Code Review)" <ge...@cloudera.org> on 2022/10/11 07:07:31 UTC

[Impala-ASF-CR] WIP IMPALA-11536: fix invalid predicates propagate for outer join simplification

Xianqing He has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19116


Change subject: WIP IMPALA-11536: fix invalid predicates propagate for outer join simplification
......................................................................

WIP IMPALA-11536: fix invalid predicates propagate for outer join simplification

When set ENABLE_OUTER_JOIN_TO_INNER_TRANSFORMATION = true, the planner
will simplify outer joins if the WHERE clause contains at least one
null rejecting condition and then will remove the outer-joined tuple id
from the map of GlobalState#outerJoinedTupleIds.
However, there may be false removals for right join simplification or
full join simplification. This may lead to incorrect results since it
is incorrect to propagate predicates into a plan subtree that is on the
nullable side of an outer join if the predicate is not null-rejecting
and determine whether it is a nullable side of an outer join according
to GlobalState#outerJoinedTupleIds.

E.g.
SELECT COUNT(*)
FROM functional.nullrows t1
  FULL JOIN functional.nullrows t2 ON t1.id = t2.id
  FULL JOIN functional.nullrows t3 ON coalesce(t1.id, t2.id) = t3.id
WHERE t1.group_str = 'a'
  AND coalesce(t2.group_str, 'f') = 'f'
The predicate coalesce(t2.group_str, 'f') = 'f' will propagate into t2
if we remove t2 from GlobalState#outerJoinedTupleIds.

Testing:
- Add new plan tests in outer-to-inner-joins.test
- Add new query tests to verify the correctness on transformation

Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test
M testdata/workloads/functional-query/queries/QueryTest/outer-to-inner-joins.test
3 files changed, 273 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/19116/4
-- 
To view, visit http://gerrit.cloudera.org:8080/19116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
Gerrit-Change-Number: 19116
Gerrit-PatchSet: 4
Gerrit-Owner: Xianqing He <he...@126.com>

[Impala-ASF-CR] IMPALA-11536: fix invalid predicates propagate for outer join simplification

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19116 )

Change subject: IMPALA-11536: fix invalid predicates propagate for outer join simplification
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9204/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
Gerrit-Change-Number: 19116
Gerrit-PatchSet: 9
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 06 Apr 2023 06:13:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11536: fix invalid predicates propagate for outer join simplification

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.

Xianqing He has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/19116 )

Change subject: IMPALA-11536: fix invalid predicates propagate for outer join simplification
......................................................................

IMPALA-11536: fix invalid predicates propagate for outer join simplification

When set ENABLE_OUTER_JOIN_TO_INNER_TRANSFORMATION = true, the planner
will simplify outer joins if the WHERE clause contains at least one
null rejecting condition and then remove the outer-joined tuple id
from the map of GlobalState#outerJoinedTupleIds.
However, there may be false removals for right join simplification or
full join simplification. This may lead to incorrect results since it
is incorrect to propagate a non null-rejecting predicate into a plan
subtree that is on the nullable side of an outer join.
GlobalState#outerJoinedTupleIds indicates whether a table is on the
nullable side of an outer join.

E.g.
SELECT COUNT(*)
FROM functional.nullrows t1
  FULL JOIN functional.nullrows t2 ON t1.id = t2.id
  FULL JOIN functional.nullrows t3 ON coalesce(t1.id, t2.id) = t3.id
WHERE t1.group_str = 'a'
  AND coalesce(t2.group_str, 'f') = 'f'
The predicate coalesce(t2.group_str, 'f') = 'f' will propagate into t2
if we remove t2 from GlobalState#outerJoinedTupleIds.

Testing:
- Add new plan tests in outer-to-inner-joins.test
- Add new query tests to verify the correctness on transformation

Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test
M testdata/workloads/functional-query/queries/QueryTest/outer-to-inner-joins.test
3 files changed, 307 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/19116/8
-- 
To view, visit http://gerrit.cloudera.org:8080/19116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
Gerrit-Change-Number: 19116
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>

[Impala-ASF-CR] IMPALA-11536: fix invalid predicates propagate for outer join simplification

Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-11536: fix invalid predicates propagate for outer join simplification
......................................................................


Patch Set 6:

(12 comments)

Looks good!

http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@11
PS6, Line 11: then
nit and then remove


http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@14
PS6, Line 14: This may lead to incorrect results since it
            : is incorrect to propagate predicates into a plan subtree that is on the
            : nullable side of an outer join if the predicate is not null-rejecting
nit. Suggest to reword to:

This may lead to incorrect results since it
is incorrect to propagate a non null-rejecting predicate into a plan subtree that is on the nullable side of an outer join.


http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@17
PS6, Line 17: and determine whether it is a nullable side of an outer join according
            : to GlobalState#outerJoinedTupleIds
nit. Suggest to reword to

GlobalState#outerJoinedTupleIds indicates whether a table is on the nullable side of an outer join.


http://gerrit.cloudera.org:8080/#/c/19116/6/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/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4093
PS6, Line 4093: nonnullableTids
nit. Suggest to rename to nullRejectingTids to make the code more readable. 

Maybe also a good idea to add a comment about this input argument.


http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4119
PS6, Line 4119: getNonnullableTids(ids, nonnullableTids);
May need to add a comment for this line of code like: find out all null-rejecting TupleIds in "ids".


http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4120
PS6, Line 4120: if (TupleId.intersect(ids, nonnullableTids) || hasNullRejectingConjucts(ids))
If getNonnullableTids(ids, nonnullableTids) at line 4119 does find a subset in 'ids' that are null-rejecting, then this line becomes un-necessary.


http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4166
PS6, Line 4166: outer joined
nit. We probably should not say "outer joined" here since they are just arguments.


http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4169
PS6, Line 4169: nonnullableTids
nit. nullRejectingTids


http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4169
PS6, Line 4169: getNonnullableTids
nit. maybe gatherNullRejectingTids().


http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4190
PS6, Line 4190: LOG.warn("Skipping to check whether the conjunct is null-rejecting because"
              :               + "backend evaluation failed: " + e.toSql(), ex);
nit. The following probably is a more straightforward statement. 

 LOG.warn("Fail to verify " + e.toSql + " being null-rejecting because of the backend evaluation failure", ex);


http://gerrit.cloudera.org:8080/#/c/19116/6/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/19116/6/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@1030
PS6, Line 1030: FROM functional.jointbl
              : FULL OUTER JOIN
              : functional.testtbl
              : ON jointbl.test_id = testtbl.id
              : FULL OUTER JOIN
These new tests with 3-way joins look good. I wonder if we can also add two more test queries that are 2-way out joins.


http://gerrit.cloudera.org:8080/#/c/19116/6/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@1065
PS6, Line 1065: correcttly
typo. double 't'.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
Gerrit-Change-Number: 19116
Gerrit-PatchSet: 6
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Oct 2022 15:05:56 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11536: fix invalid predicates propagate for outer join simplification

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.

Xianqing He has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/19116 )

Change subject: IMPALA-11536: fix invalid predicates propagate for outer join simplification
......................................................................

IMPALA-11536: fix invalid predicates propagate for outer join simplification

When set ENABLE_OUTER_JOIN_TO_INNER_TRANSFORMATION = true, the planner
will simplify outer joins if the WHERE clause contains at least one
null rejecting condition and then will remove the outer-joined tuple id
from the map of GlobalState#outerJoinedTupleIds.
However, there may be false removals for right join simplification or
full join simplification. This may lead to incorrect results since it
is incorrect to propagate predicates into a plan subtree that is on the
nullable side of an outer join if the predicate is not null-rejecting
and determine whether it is a nullable side of an outer join according
to GlobalState#outerJoinedTupleIds.

E.g.
SELECT COUNT(*)
FROM functional.nullrows t1
  FULL JOIN functional.nullrows t2 ON t1.id = t2.id
  FULL JOIN functional.nullrows t3 ON coalesce(t1.id, t2.id) = t3.id
WHERE t1.group_str = 'a'
  AND coalesce(t2.group_str, 'f') = 'f'
The predicate coalesce(t2.group_str, 'f') = 'f' will propagate into t2
if we remove t2 from GlobalState#outerJoinedTupleIds.

Testing:
- Add new plan tests in outer-to-inner-joins.test
- Add new query tests to verify the correctness on transformation

Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test
M testdata/workloads/functional-query/queries/QueryTest/outer-to-inner-joins.test
3 files changed, 286 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/19116/6
-- 
To view, visit http://gerrit.cloudera.org:8080/19116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
Gerrit-Change-Number: 19116
Gerrit-PatchSet: 6
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-11536: fix invalid predicates propagate for outer join simplification

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19116 )

Change subject: IMPALA-11536: fix invalid predicates propagate for outer join simplification
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12610/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7
Gerrit-Change-Number: 19116
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qf...@hotmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Mon, 13 Mar 2023 08:24:20 +0000
Gerrit-HasComments: No