Skip to content

Better optimization of consecutive keys in aggregation - #57872

Merged
alexey-milovidov merged 6 commits into
ClickHouse:masterfrom
CurtizJ:optimize-aggregation-consecutive-keys
Dec 27, 2023
Merged

alexey-milovidov merged 6 commits into
ClickHouse:masterfrom
CurtizJ:optimize-aggregation-consecutive-keys

Conversation

@CurtizJ

@CurtizJ CurtizJ commented Dec 14, 2023 •

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Performance Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Optimized aggregation in some cases.

Detailed description:

There are two optimizations in this PR:

  1. Execute addBatchSinglePlace instead of addBatch in case when we have only one unique key in block.
  2. Disable consecutive keys cache in case when miss rate is high.

@robot-ch-test-poll robot-ch-test-poll added the pr-performance Pull request with some performance improvements label Dec 14, 2023
@robot-ch-test-poll

robot-ch-test-poll commented Dec 14, 2023 •

Copy link
Copy Markdown
Contributor

This is an automated comment for commit 27bcdbe with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

Successful checks
Check nameDescriptionStatus
Docs checkThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Mergeable CheckChecks if all other necessary checks are successful✅ success
Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending

@CurtizJ CurtizJ changed the title Optimize aggregation by large ranges of equal keys (second try) Better optimization of consecutive keys in aggregation Dec 15, 2023
@CurtizJ
CurtizJ force-pushed the optimize-aggregation-consecutive-keys branch from a4adabf to 5faf5e9 Compare December 16, 2023 03:36
@CurtizJ

CurtizJ commented Dec 16, 2023

Copy link
Copy Markdown
Member Author

Some synthetic perf tests sped up a bit:

Screenshot 2023-12-16 at 16 20 27 Screenshot 2023-12-16 at 16 20 51 Screenshot 2023-12-16 at 16 21 05

@CurtizJ
CurtizJ force-pushed the optimize-aggregation-consecutive-keys branch from 904b764 to 10bc2db Compare December 18, 2023 13:57
@CurtizJ
CurtizJ marked this pull request as ready for review December 18, 2023 16:50
@antonio2368 antonio2368 self-assigned this Dec 19, 2023
bool check(const Key & key) const { return value.first == key; }

bool hasOnlyOneValue() const { return found && misses == 1; }
UInt64 getMisses() const { return misses; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of getter if fields are public and they don't have getters?

Comment thread src/Core/Settings.h Resolved
Co-authored-by: Antonio Andelic <[email protected]>

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing!

@alexey-milovidov alexey-milovidov self-assigned this Dec 27, 2023
@alexey-milovidov
alexey-milovidov merged commit f00337e into ClickHouse:master Dec 27, 2023
baibaichen added a commit to Kyligence/gluten that referenced this pull request Dec 28, 2023
baibaichen added a commit to Kyligence/gluten that referenced this pull request Dec 28, 2023
pull Bot pushed a commit to Lobinson1/ClickHouse that referenced this pull request Jul 30, 2026
TTLAggregationAlgorithm built its Aggregator::Params with
min_chunk_bytes_for_parallel_parsing in the argument slot that expects
min_hit_rate_to_use_consecutive_keys_optimization. The former is a byte
threshold defaulting to 10 MiB, the latter a hit rate in [0, 1] defaulting
to 0.5, so the threshold arrived as 10485760.0.

Aggregator::executeImpl compares cache_hit_rate (at most 1.0) against that
threshold, so use_cache was always false, Method::StateNoCache was selected,
and StateImpl<false>::hasOnlyOneValueSinceLastReset() returns false
unconditionally. That value gates the equal-key-range batching in
executeAggregateInstructions, so addBatchSinglePlace was never taken and the
consecutive keys optimization was permanently disabled for TTL GROUP BY
aggregation. This is the path that benefits most from it: execute() calls
calculateAggregates() once per run of equal keys, so every executeOnBlock
receives a single-key chunk.

Introduced by ClickHouse#57872, which added the parameter and used the correct setting
at every other call site; the narrowing conversion that would have flagged it
was later silenced by the static_cast<float> from ClickHouse#94643. Present since
v23.12.1.1368-stable. Results are not affected, only performance.

There is no parallel parsing in this file, so the now-unused
min_chunk_bytes_for_parallel_parsing extern is removed.

Measured on the added test (200000 rows, 100 keys, one aggregating TTL
merge): ProfileEvents['AggregationOptimizedEqualRangesOfKeys'] goes from 0
to 588 while count() and sum(v) stay identical.

Closes: ClickHouse#112075
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-performance Pull request with some performance improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants