You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2025/01/09 23:42:00 UTC
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22322
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
IMPALA-13658: Enable tuple caching aggregates
Enables tuple caching on aggregates. Caching aggregates requires that
their children are also eligible for caching, so this excludes
aggregates above an exchange.
Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
3 files changed, 92 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/22322/1
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22322/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/1//COMMIT_MSG@9
PS1, Line 9: Enables tuple caching on aggregates. Caching aggregates requires that
Which aggregates are eligible? Should be any aggregate that does not involve an exchange including pre-aggregates and single-threaded final aggregates.
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
File fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java:
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@227
PS1, Line 227: }
Should we test ineligible grouping and aggregate expressions?
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 10 Jan 2025 15:38:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
File fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java:
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@227
PS1, Line 227: }
> The way our testing currently works, I'm verifying caching by counting the
I'll add some comments about which aggregates are not expected in each test case too.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 10 Jan 2025 21:51:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18002/ : 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/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 22:01:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22322/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/4//COMMIT_MSG@12
PS4, Line 12:
Please mention preliminary performance results. We should be seeing massive benefits from caching aggregates.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 16:47:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 5: Code-Review+1
Thanks for excluding appx_median, this is looking good to me
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 17 Jan 2025 00:14:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18022/ : 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/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 23:08:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 4:
> Patch Set 4:
>
> (1 comment)
>
> > Patch Set 3:
> >
> > (1 comment)
> >
> > This makes sense to me.
> >
> > One thing: The appx_median aggregation function is non-deterministic. That isn't necessarily a problem when returned directly to users. It could be a problem if it feeds into the build side of a join or influences a runtime filter at a location that is cached.
>
> Do we have a mechanism to mark that already? If not, I'd file it as a follow-on ticket.
Should be able to add it to isNondeterministicAcrossQueries.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 19:04:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
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/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
IMPALA-13658: Enable tuple caching aggregates
Enables tuple caching on aggregates directly above scan nodes. Caching
aggregates requires that their children are also eligible for caching,
so this excludes aggregates above an exchange, union, or hash join.
Testing:
- Adds Planner tests for different aggregate cases to confirm they have
stable tuple cache keys and are valid for caching.
- Adds custom cluster tests that cached aggregates are used, and can be
re-used in slightly different statements.
Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Reviewed-on: http://gerrit.cloudera.org:8080/22322
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
4 files changed, 139 insertions(+), 7 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Kurt Deschler, Yida Wu, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22322
to look at the new patch set (#2).
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
IMPALA-13658: Enable tuple caching aggregates
Enables tuple caching on aggregates directly above scan nodes. Caching
aggregates requires that their children are also eligible for caching,
so this excludes aggregates above an exchange, union, or hash join.
Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
3 files changed, 110 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/22322/2
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Kurt Deschler, Yida Wu, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22322
to look at the new patch set (#4).
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
IMPALA-13658: Enable tuple caching aggregates
Enables tuple caching on aggregates directly above scan nodes. Caching
aggregates requires that their children are also eligible for caching,
so this excludes aggregates above an exchange, union, or hash join.
Testing:
- Adds Planner tests for different aggregate cases to confirm they have
stable tuple cache keys and are valid for caching.
- Adds custom cluster tests that cached aggregates are used, and can be
re-used in slightly different statements.
Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
3 files changed, 134 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/22322/4
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Jan 2025 17:50:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 3:
(1 comment)
This makes sense to me.
One thing: The appx_median aggregation function is non-deterministic. That isn't necessarily a problem when returned directly to users. It could be a problem if it feeds into the build side of a join or influences a runtime filter at a location that is cached.
http://gerrit.cloudera.org:8080/#/c/22322/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22322/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@695
PS3, Line 695: msg.agg_node.setEstimated_input_cardinality(getChild(0).getCardinality());
Nit: We could exclude this to avoid variability if some stats change.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 20:51:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 5:
I'll kick off a precommit run
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Jan 2025 17:53:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Jan 2025 17:53:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Kurt Deschler, Yida Wu, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22322
to look at the new patch set (#3).
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
IMPALA-13658: Enable tuple caching aggregates
Enables tuple caching on aggregates directly above scan nodes. Caching
aggregates requires that their children are also eligible for caching,
so this excludes aggregates above an exchange, union, or hash join.
Testing:
- Adds Planner tests for different aggregate cases to confirm they have
stable tuple cache keys and are valid for caching.
- Adds custom cluster tests that cached aggregates are used, and can be
re-used in slightly different statements.
Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
3 files changed, 131 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/22322/3
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 3:
(1 comment)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22322/3/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:
http://gerrit.cloudera.org:8080/#/c/22322/3/tests/custom_cluster/test_tuple_cache.py@388
PS3, Line 388: d
flake8: E303 too many blank lines (2)
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 19:59:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 4:
(1 comment)
> Patch Set 3:
>
> (1 comment)
>
> This makes sense to me.
>
> One thing: The appx_median aggregation function is non-deterministic. That isn't necessarily a problem when returned directly to users. It could be a problem if it feeds into the build side of a join or influences a runtime filter at a location that is cached.
Do we have a mechanism to mark that already? If not, I'd file it as a follow-on ticket.
http://gerrit.cloudera.org:8080/#/c/22322/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22322/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@695
PS3, Line 695: // Normalize input cardinality estimate for caching in case stats change.
> Nit: We could exclude this to avoid variability if some stats change.
Done
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 21:36:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22322/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/1//COMMIT_MSG@9
PS1, Line 9: Enables tuple caching on aggregates. Caching aggregates requires that
> Which aggregates are eligible? Should be any aggregate that does not involv
I thought the next sentence covered that. I can try to reword a bit to match how you described it.
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
File fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java:
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@227
PS1, Line 227: }
> Should we test ineligible grouping and aggregate expressions?
The way our testing currently works, I'm verifying caching by counting the number of tuple cache nodes. Some of that testing is implicit in the counts above in that they might be multi-phase aggregates but only 2 tuple cache nodes (above scan and first aggregate).
I'll add tests:
- where there's no aggregate below an exchange
- where the aggregate is above a union
I could also look into making the test verification more specific, but I don't have concrete ideas about that yet.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 10 Jan 2025 21:46:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11354/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Jan 2025 17:53:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Kurt Deschler, Yida Wu, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22322
to look at the new patch set (#5).
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
IMPALA-13658: Enable tuple caching aggregates
Enables tuple caching on aggregates directly above scan nodes. Caching
aggregates requires that their children are also eligible for caching,
so this excludes aggregates above an exchange, union, or hash join.
Testing:
- Adds Planner tests for different aggregate cases to confirm they have
stable tuple cache keys and are valid for caching.
- Adds custom cluster tests that cached aggregates are used, and can be
re-used in slightly different statements.
Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
---
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
M tests/custom_cluster/test_tuple_cache.py
4 files changed, 139 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/22322/5
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22322/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/2//COMMIT_MSG@12
PS2, Line 12:
> Could we also mention the new added test cases in the testing section?
Done
http://gerrit.cloudera.org:8080/#/c/22322/2/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:
http://gerrit.cloudera.org:8080/#/c/22322/2/tests/custom_cluster/test_tuple_cache.py@382
PS2, Line 382: bytes_read = getCounterValues(result2.runtime_profile, "TupleCacheBytesRead")
> Would it be good to add two more simple cases after the previous two querie
Done
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 19:57:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18001/ : 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/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 20:23:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22322/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/1//COMMIT_MSG@9
PS1, Line 9: Enables tuple caching on aggregates directly above scan nodes. Caching
> I thought the next sentence covered that. I can try to reword a bit to matc
Done
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
File fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java:
http://gerrit.cloudera.org:8080/#/c/22322/1/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@227
PS1, Line 227: verifyCacheIneligible(
> I'll add some comments about which aggregates are not expected in each test
Done
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 11 Jan 2025 00:21:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17950/ : 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/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 11 Jan 2025 00:48:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Jan 2025 23:26:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17931/ : 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/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 10 Jan 2025 00:09:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Looks good.
Tested with tpc queries with correctness verification on, no issues found.
http://gerrit.cloudera.org:8080/#/c/22322/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/2//COMMIT_MSG@12
PS2, Line 12:
Could we also mention the new added test cases in the testing section?
http://gerrit.cloudera.org:8080/#/c/22322/2/tests/custom_cluster/test_tuple_cache.py
File tests/custom_cluster/test_tuple_cache.py:
http://gerrit.cloudera.org:8080/#/c/22322/2/tests/custom_cluster/test_tuple_cache.py@382
PS2, Line 382: assert bytes_written[0] == bytes_read[0]
Would it be good to add two more simple cases after the previous two queries: "select avg(int_col) from functional.alltypes;" and "select avg(a) from (select sum(int_col) as a from functional.alltypes) b;"? The first case would test loading the cache from the scan node, while the second case would test loading the cache from the sum agg node with a slightly different query.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 15 Jan 2025 10:04:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13658: Enable tuple caching aggregates
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22322 )
Change subject: IMPALA-13658: Enable tuple caching aggregates
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22322/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22322/4//COMMIT_MSG@12
PS4, Line 12:
> Please mention preliminary performance results. We should be seeing massive
I can get some small numbers with my local setup:
- TPC-H Q1 saves ~10ms out of 130ms
- TPC-DS Q9 skips all the scans (estimated 288k rows each), taking 232ms instead of 932ms.
Running https://jenkins.impala.io/job/perf-AB-test-ub2004-tuplecache/8/ for a larger comparison.
http://gerrit.cloudera.org:8080/#/c/22322/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java
File fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java:
http://gerrit.cloudera.org:8080/#/c/22322/5/fe/src/test/java/org/apache/impala/planner/TupleCacheTest.java@166
PS5, Line 166: String appxMedianAgg = "select appx_median(tinyint_col) from functional.alltypesagg";
Excluded appx_median, test here shows it works.
--
To view, visit http://gerrit.cloudera.org:8080/22322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9bd13c2813c90d23eb3a70f98068fdcdab97a885
Gerrit-Change-Number: 22322
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 22:48:51 +0000
Gerrit-HasComments: Yes