Add support for adaptive partial aggregation by prithvip · Pull Request #20979 · prestodb/presto

prithvip · 2023-09-28T02:17:00Z

When partial aggregation is not effectively reducing cardinality, instead send raw input rows directly to the final aggregation step.

Port of github.com/trinodb/trino/pull/11011 and github.com/trinodb/trino/pull/17143

Description

This change adds support for adaptive partial aggregation.

Motivation and Context

Partial Aggregation can be inefficient when the number of unique values is high and the buffer is too small to effectively reduce cardinality. In these cases, the wasted CPU from hashing exceeds the benefit of transmitting less rows over the network. Partial aggregation should be disabled in this case.

Impact

Benchmarks results on TPCH:

Test Plan

Unit tests
Ran Presto cluster on extended shadow with this feature enabled
Verifier run on production workload

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==
General Changes
* Improve efficiency of partial aggregation step. 
  This can be enabled by setting the ``adaptive_partial_aggregation`` session property to true.

linux-foundation-easycla · 2023-09-28T02:17:06Z

✅ login: prithvip (c2312bb)
❌ - login: @lukasz-stec / name: Lukasz Stec . The commit (c2312bb) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

agrawaldevesh · 2023-09-28T04:00:58Z

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

are these configs same as the ones used in Trino ?

Yes they are

agrawaldevesh · 2023-09-28T04:02:13Z

presto-main/src/main/java/com/facebook/presto/operator/CompletedWork.java

Should we add a comment on what a null result means ?

agrawaldevesh · 2023-09-28T04:03:56Z

presto-main/src/main/java/com/facebook/presto/operator/HashAggregationOperator.java

nit: Will hash agg ever produce non unique rows ? ie, Is the word unique redundant ?

Yes, it will always be unique, but I think its clearer to call it unique to understand the logic, instead of something more generic like outputRowsProduced

agrawaldevesh · 2023-09-28T04:05:30Z

presto-main/src/main/java/com/facebook/presto/operator/HashAggregationOperator.java

nit: Add a comment on what an empty uniquerowsproduced means and how it differs from 0 unique rows.

agrawaldevesh · 2023-09-28T04:07:38Z

presto-main/src/main/java/com/facebook/presto/operator/HashAggregationOperator.java

Do we need any special handling in this clause wrt to spillEnabled ?

+1. looking at the code with spillEnabled we are bypassing it and going with Skip . Would that cause local OOMs?

Whether spill is enabled or not, if partial aggregation is not effective, we should use SkipAggregationBuilder instead. It would not cause local OOM since SkipAggregationBuilder doesn't use any additional memory other than the already allocated partial agg output buffer.

agrawaldevesh · 2023-09-28T04:13:00Z

...n/src/main/java/com/facebook/presto/operator/aggregation/partial/SkipAggregationBuilder.java

is this comment superfluous ?

We can remove.

Removed. But will note that there are tons of places where we have no op as single line comment

agrawaldevesh · 2023-09-28T04:14:58Z

...n/src/main/java/com/facebook/presto/operator/aggregation/partial/SkipAggregationBuilder.java

fillConsecutive ? or Range to borrow the python name ?

renamed to fillConsecutive

agrawaldevesh · 2023-09-28T04:15:50Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

Q: What is the policy b/w the feature config name and the session prop name agreeing on the prefix experimental ?

There is no policy - session prop name is totally different from feature config name and session props do not have prefixes unto themselves. When they do have prefix, it's to refer to the catalog to which the session prop should be applied.

agrawaldevesh · 2023-09-28T04:17:33Z

...main/java/com/facebook/presto/operator/aggregation/partial/PartialAggregationController.java

Why should the partial agg disabling look at the byte size instead of just the input and output row cardinalities ?

+1. It would be nice to clarify this in a comment

Also I would refactor this check totalBytesProcessed >= maxPartialMemory.toBytes() * ENABLE_AGGREGATION_BUFFER_SIZE_TO_INPUT_BYTES_RATIO) to a shouldEnablePartialAggregation similar to the disable method below.

I've added a comment clarifying why we look at # bytes as the floor instead of # of rows.

agrawaldevesh · 2023-09-28T04:18:06Z

presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java

Can you think of more tests for this ?

For example, anything to verify that the "vacous group-by" results output by the partial agg into the output buffer are consumable by the final agg for different data types ?

We do test several data types in the unit tests. For comprehensiveness, I think this is better suited for verifier testing, which will test wide range of data types

ajaygeorge · 2023-11-27T14:44:30Z

Starting to take a look today.
Please mention the properties need to enable this feature in Release notes section.

steveburnett · 2023-11-27T15:02:42Z

Starting to take a look today. Please mention the properties need to enable this feature in Release notes section.

Should this be documented, possibly in Aggregate Functions?

ajaygeorge

Looks mostly good.
Adding some comments

ajaygeorge · 2023-11-27T18:08:14Z

...main/java/com/facebook/presto/operator/aggregation/partial/PartialAggregationController.java

nit : maxPartialAggregationMemory

renamed to maxPartialAggregationMemorySize

ajaygeorge · 2023-11-27T18:11:58Z

...main/java/com/facebook/presto/operator/aggregation/partial/PartialAggregationController.java

nit considering to disable -> considering disabling

ajaygeorge · 2023-11-27T18:15:21Z

...main/java/com/facebook/presto/operator/aggregation/partial/PartialAggregationController.java

+1. It would be nice to clarify this in a comment

ajaygeorge · 2023-11-27T18:15:52Z

...main/java/com/facebook/presto/operator/aggregation/partial/PartialAggregationController.java

same as above. why bytes is important to look at?

Comment should address this here too, it is the same reason.

ajaygeorge · 2023-11-27T18:20:30Z

presto-main/src/main/java/com/facebook/presto/operator/HashAggregationOperator.java

+1. looking at the code with spillEnabled we are bypassing it and going with Skip . Would that cause local OOMs?

ajaygeorge · 2023-11-27T18:25:47Z

...main/java/com/facebook/presto/operator/aggregation/partial/PartialAggregationController.java

Also I would refactor this check totalBytesProcessed >= maxPartialMemory.toBytes() * ENABLE_AGGREGATION_BUFFER_SIZE_TO_INPUT_BYTES_RATIO) to a shouldEnablePartialAggregation similar to the disable method below.

ajaygeorge · 2023-11-27T18:30:15Z

...n/src/main/java/com/facebook/presto/operator/aggregation/partial/SkipAggregationBuilder.java

We can remove.

ajaygeorge · 2023-11-27T18:32:01Z

...n/src/main/java/com/facebook/presto/operator/aggregation/partial/SkipAggregationBuilder.java

add requireNonNull(accumulatorFactories, "accumulatorFactories is null"); before this line

ajaygeorge · 2023-11-27T18:36:17Z

...n/src/main/java/com/facebook/presto/operator/aggregation/partial/SkipAggregationBuilder.java

nit. prefer functional style. (inputHashChannel.map(size -> 1).orElse(0))

feels clearer to me to leave as is

ajaygeorge · 2023-11-27T18:45:02Z

...n/src/main/java/com/facebook/presto/operator/aggregation/partial/SkipAggregationBuilder.java

can you help me understand why were are using NOOP here.? Is it because we are not doing any aggregation and just returning the flat rows.?

Yes exactly, there is no memory used by this builder

ajaygeorge

LGTM % a few comments.

When partial aggregation is not effectively reducing cardinality, instead send raw input rows directly to the final aggregation step. Port of: github.com/trinodb/trino/pull/11011 github.com/trinodb/trino/pull/17143 Co-authored-by: Lukasz Stec <lukasz.s.stec@gmail.com> Co-authored-by: Karol Sobczak <sopel39@users.noreply.github.com>

prithvip · 2023-11-27T22:36:38Z

Starting to take a look today. Please mention the properties need to enable this feature in Release notes section.

I've added session property info to enable this in Release Notes section

prithvip requested a review from a team as a code owner September 28, 2023 02:17

prithvip requested a review from presto-oss September 28, 2023 02:17

prithvip requested review from MnO2 and ajaygeorge September 28, 2023 03:35

agrawaldevesh reviewed Sep 28, 2023

View reviewed changes

ajaygeorge reviewed Nov 27, 2023

View reviewed changes

ajaygeorge approved these changes Nov 27, 2023

View reviewed changes

prithvip force-pushed the adapative-partial-agg branch from e1ede28 to c2312bb Compare November 27, 2023 21:47

ajaygeorge approved these changes Nov 27, 2023

View reviewed changes

prithvip merged commit 588660e into prestodb:master Nov 27, 2023

wanglinsong mentioned this pull request Feb 12, 2024

Add release notes for 0.286 #21906

Merged

64 tasks

Conversation

prithvip commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

linux-foundation-easycla bot commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaygeorge commented Nov 27, 2023

Uh oh!

steveburnett commented Nov 27, 2023

Uh oh!

ajaygeorge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prithvip commented Sep 28, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 28, 2023 •

edited

Loading