You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2017/12/07 22:44:12 UTC
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8791
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
When a sort is inserted into a plan for an INSERT due to either the
target table being a Kudu table or the use of the 'clustered' hint,
and a TupleIsNullPredicate is present in the output of the sort, the
TupleIsNullPredicate may reference an incorrect tuple (i.e. not the
materialized sort tuple), leading to errors.
The solution is to materialize the TupleIsNullPredicate into the sort
tuple and then perform the appropriate expr substitutions, as is
already done for the case of analytic sorts.
Testing:
- Added an e2e test with a query that would previously fail.
Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
---
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
3 files changed, 46 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/8791/1
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 23:58:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8791/2/testdata/workloads/functional-query/queries/QueryTest/insert.test
File testdata/workloads/functional-query/queries/QueryTest/insert.test:
http://gerrit.cloudera.org:8080/#/c/8791/2/testdata/workloads/functional-query/queries/QueryTest/insert.test@949
PS2, Line 949: # IMPALA-6280: clustered with outer join, inline view, and TupleisNullPredicate
Are you sure this test is executed even without a RESULTS section?
http://gerrit.cloudera.org:8080/#/c/8791/2/testdata/workloads/functional-query/queries/QueryTest/insert.test@954
PS2, Line 954: right outer join functional.alltypes t1 on t1.id = v.id
let's use alltypestiny for t1
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 19:38:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
File fe/src/main/java/org/apache/impala/analysis/SortInfo.java:
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@195
PS1, Line 195: List<Expr> substResultExprs =
> Let's create a list like this:
Done
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@255
PS1, Line 255: /**
> Comment sounds a little cluttered and some references like "the original ex
Done
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@262
PS1, Line 262: List<Expr> exprs, List<Expr> sortSlotExprs, ExprSubstitutionMap substOrderBy,
> TreeNode.collect() like in L197
Done
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java:
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@287
PS1, Line 287: List<Expr> relevantRhsExprs = Lists.newArrayList();
> relevantRhsExprs
Done
http://gerrit.cloudera.org:8080/#/c/8791/1/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test:
http://gerrit.cloudera.org:8080/#/c/8791/1/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test@391
PS1, Line 391: # IMPALA-6280: check that TupleIsNullPredicate is materialized correctly
> Good catch with the clustered hint, please modify the JIRA description to m
Done
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 18:50:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1599/
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 20:23:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8791
to look at the new patch set (#2).
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
When a sort is inserted into a plan for an INSERT due to either the
target table being a Kudu table or the use of the 'clustered' hint,
and a TupleIsNullPredicate is present in the output of the sort, the
TupleIsNullPredicate may reference an incorrect tuple (i.e. not the
materialized sort tuple), leading to errors.
The solution is to materialize the TupleIsNullPredicate into the sort
tuple and then perform the appropriate expr substitutions, as is
already done for the case of analytic sorts.
Testing:
- Added an e2e test with a query that would previously fail.
Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
---
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-query/queries/QueryTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
4 files changed, 57 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/8791/2
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8791
to look at the new patch set (#3).
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
When a sort is inserted into a plan for an INSERT due to either the
target table being a Kudu table or the use of the 'clustered' hint,
and a TupleIsNullPredicate is present in the output of the sort, the
TupleIsNullPredicate may reference an incorrect tuple (i.e. not the
materialized sort tuple), leading to errors.
The solution is to materialize the TupleIsNullPredicate into the sort
tuple and then perform the appropriate expr substitutions, as is
already done for the case of analytic sorts.
Testing:
- Added an e2e test with a query that would previously fail.
Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
---
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-query/queries/QueryTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
4 files changed, 60 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/8791/3
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
When a sort is inserted into a plan for an INSERT due to either the
target table being a Kudu table or the use of the 'clustered' hint,
and a TupleIsNullPredicate is present in the output of the sort, the
TupleIsNullPredicate may reference an incorrect tuple (i.e. not the
materialized sort tuple), leading to errors.
The solution is to materialize the TupleIsNullPredicate into the sort
tuple and then perform the appropriate expr substitutions, as is
already done for the case of analytic sorts.
Testing:
- Added an e2e test with a query that would previously fail.
Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Reviewed-on: http://gerrit.cloudera.org:8080/8791
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-query/queries/QueryTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
4 files changed, 60 insertions(+), 15 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 20:21:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8791/2/testdata/workloads/functional-query/queries/QueryTest/insert.test
File testdata/workloads/functional-query/queries/QueryTest/insert.test:
http://gerrit.cloudera.org:8080/#/c/8791/2/testdata/workloads/functional-query/queries/QueryTest/insert.test@949
PS2, Line 949: # IMPALA-6280: clustered with outer join, inline view, and TupleisNullPredicate
> Are you sure this test is executed even without a RESULTS section?
Yes, but probably better to add one anyways
http://gerrit.cloudera.org:8080/#/c/8791/2/testdata/workloads/functional-query/queries/QueryTest/insert.test@954
PS2, Line 954: right outer join functional.alltypestiny t1 on t1.id = v.id
> let's use alltypestiny for t1
Done
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 20:20:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8791 )
Change subject: IMPALA-6280: Materialize TupleIsNullPredicate for insert sorts
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java
File fe/src/main/java/org/apache/impala/analysis/SortInfo.java:
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@195
PS1, Line 195: TreeNode.collect(Expr.substituteList(resultExprs, substOrderBy, analyzer, false),
Let's create a list like this:
List<Expr> substResultExprs = Expr.substituteList(resultExprs, substOrderBy, analyzer, false);
Which we can also pass in L211. Passing resultExprs in L211 is correct, but it includes some exprs we don't need.
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@255
PS1, Line 255: * Materializes any TupleIsNullPredicates present in 'exprs' into slots in
Comment sounds a little cluttered and some references like "the original expr" are not clear. Might be clearer to do something like this (but I'm open to other cleanup if you prefer that):
Collects the unique TupleIsNullPredicates from 'exprs' for each one:
* creates a new slot in ...
* adds the TupleIsNullPredicate to ...
* adds a entry in ... mapping from the TupleIsNullPredicate to a SlotRef into the new slot
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@262
PS1, Line 262: List<Expr> tupleIsNullPredsToMaterialize = Lists.newArrayList();
TreeNode.collect() like in L197
can also call this tupleIsNullPreds (it's clear we're going to materialize all of the unique ones)
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java:
http://gerrit.cloudera.org:8080/#/c/8791/1/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@287
PS1, Line 287: List<Expr> possibleTupleIsNullPreds = Lists.newArrayList();
relevantRhsExprs
http://gerrit.cloudera.org:8080/#/c/8791/1/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test:
http://gerrit.cloudera.org:8080/#/c/8791/1/testdata/workloads/functional-query/queries/QueryTest/kudu_upsert.test@391
PS1, Line 391: # IMPALA-6280: check that TupleIsNullPredicate is materialized correctly
Good catch with the clustered hint, please modify the JIRA description to mention that case.
Also add a non-Kudu test for the clustered hint.
--
To view, visit http://gerrit.cloudera.org:8080/8791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6c0ca717aa4321a5cc84edd1d5857912f8c85583
Gerrit-Change-Number: 8791
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Dec 2017 00:20:52 +0000
Gerrit-HasComments: Yes