{{ message }}
Sharded Aggregation for high cardinality data#99581
Open
nihalzp wants to merge 108 commits into
Open
Conversation
Contributor
clickhouse-gh
Bot
added
the
pr-performance
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| for (size_t i = 0; i < num_rows; ++i) | ||
| { | ||
| size_t row = row_indices[i]; | ||
| size_t current_offset = offsets[static_cast<ssize_t>(row) - 1]; |
Contributor
There was a problem hiding this comment.
❌ addBatchArrayForRows reads offsets[row - 1] without handling row == 0.
row_indices can include the first row of the chunk, so for row = 0 this becomes offsets[-1], which is an out-of-bounds read (undefined behavior).
Please guard the first-row case explicitly, e.g.:
size_t current_offset = (row == 0) ? 0 : offsets[row - 1];
1 task
| throw Exception( | ||
| ErrorCodes::LOGICAL_ERROR, | ||
| "Sharded aggregation requires prealloc serialized methods, but row_sizes is empty. " | ||
| "Sharded aggregation should be avoided for non-prealloc methods should be in AggregatingStep"); |
Contributor
There was a problem hiding this comment.
💡 The exception text has a grammar typo and reads awkwardly:
Sharded aggregation should be avoided for non-prealloc methods should be in AggregatingStep
Please reword it to something clearer, for example:
Sharded aggregation should be avoided for non-prealloc methods in AggregatingStep.
Member
|
Our MVP grew into a MegaVP. Let's break it into a few steps that will be easier to review and discuss in detail. Here is the plan we came up with: I'd perhaps limit ourselves to 4 or even 3 stages instead of 6, but it is up to you. My only big ask is to move iteratively. |
|
|
||
| - [GROUP BY optimization](/sql-reference/statements/select/group-by#group-by-optimization-depending-on-table-sorting-key) | ||
| )", 0) \ | ||
| DECLARE(Bool, optimize_aggregation_by_sharding, true, R"( |
Contributor
There was a problem hiding this comment.
optimize_aggregation_by_sharding) with new behavior, but the PR still has the "Documentation is written" checkbox unchecked.
Please add user docs for this setting (when to enable it, limitations such as lack of external aggregation spill support, and expected trade-offs), then mark the template checkbox accordingly.
Contributor
LLVM Coverage Report
Changed lines: 80.38% (762/948) | lost baseline coverage: 15 line(s) · Uncovered code |
1 task
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
New
GROUP BYoptimization for high cardinality keys that distributes rows across threads by hashing the grouping key, so each thread aggregates a disjoint subset of keys without a merge phase. Setoptimize_aggregation_by_sharding = 1to enable it.Documentation entry for user-facing changes