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 2018/02/05 22:29:07 UTC
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9218
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Previously, an analytic function that used the same expr in both the
'partition by' and 'order by' clauses, and where the expr meets the
criteria for being materialized before the sort, would hit an
IllegalStateException due to the expr being inserted into the same
ExprSubstitutionMap twice.
If the values have already been partitioned on the expr, then all of
the values for it in each partition will be the same and also ordering
on the expr doesn't change the results. So, the fix is to simply
exclude the duplicate expr from the 'order by' while still
partitioning on it.
Testing:
- Added a regression test to analytic-fns.test
Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
---
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
2 files changed, 24 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/9218/1
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9218 )
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1888/
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
Gerrit-PatchSet: 2
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: Mon, 05 Feb 2018 23:43:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9218 )
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
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: Mon, 05 Feb 2018 23:16:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/9218 )
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9218/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/9218/1/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@342
PS1, Line 342: // If the expr is in the PARTITION BY and already in 'sortExprs', but also in
> minor suggestion:
Done
http://gerrit.cloudera.org:8080/#/c/9218/1/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
File testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test:
http://gerrit.cloudera.org:8080/#/c/9218/1/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@2086
PS1, Line 2086:
> Since the bug was in the planner, please make this a planner test.
Done
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
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: Mon, 05 Feb 2018 23:15:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
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/9218 )
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Previously, an analytic function that used the same expr in both the
'partition by' and 'order by' clauses, and where the expr meets the
criteria for being materialized before the sort, would hit an
IllegalStateException due to the expr being inserted into the same
ExprSubstitutionMap twice.
If the values have already been partitioned on the expr, then all of
the values for it in each partition will be the same and also ordering
on the expr doesn't change the results. So, the fix is to simply
exclude the duplicate expr from the 'order by' while still
partitioning on it.
Testing:
- Added a regression test to PlannerTest.
Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Reviewed-on: http://gerrit.cloudera.org:8080/9218
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
3 files changed, 33 insertions(+), 6 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
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>
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9218 )
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
Gerrit-PatchSet: 2
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: Tue, 06 Feb 2018 03:25:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
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/9218
to look at the new patch set (#2).
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Previously, an analytic function that used the same expr in both the
'partition by' and 'order by' clauses, and where the expr meets the
criteria for being materialized before the sort, would hit an
IllegalStateException due to the expr being inserted into the same
ExprSubstitutionMap twice.
If the values have already been partitioned on the expr, then all of
the values for it in each partition will be the same and also ordering
on the expr doesn't change the results. So, the fix is to simply
exclude the duplicate expr from the 'order by' while still
partitioning on it.
Testing:
- Added a regression test to PlannerTest.
Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
---
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
3 files changed, 33 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/9218/2
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-6473: Fix analytic fn that partitions and orders on same expr
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9218 )
Change subject: IMPALA-6473: Fix analytic fn that partitions and orders on same expr
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9218/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/9218/1/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@342
PS1, Line 342: // If the expr is in the 'partition by' and already in 'sortExprs', but also in
minor suggestion:
Exprs that appear in both PARTITION BY and ORDER BY only need to be added to the 'sortExprs' once.
http://gerrit.cloudera.org:8080/#/c/9218/1/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test
File testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test:
http://gerrit.cloudera.org:8080/#/c/9218/1/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@2086
PS1, Line 2086: # IMPALA-6473: analytic fn where the same expr is in the 'partition by' and the 'order by'
Since the bug was in the planner, please make this a planner test.
--
To view, visit http://gerrit.cloudera.org:8080/9218
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id5f1d5fbc6f69df5850f96afed345ce27668c30b
Gerrit-Change-Number: 9218
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Feb 2018 22:36:55 +0000
Gerrit-HasComments: Yes