executor: Improve HashAgg string collation key caching - #10978
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds CI collation sort-key caching to string hash aggregation, tracks persistent fallback after cache thresholds are exceeded, wires initialization and status propagation through ChangesCollation sort-key caching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Aggregator
participant HashMethodString
participant Collator
participant AggregatedDataVariants
Aggregator->>AggregatedDataVariants: check fallback state
Aggregator->>HashMethodString: initialize cache for batch
HashMethodString->>Collator: generate or reuse sort keys
HashMethodString-->>Aggregator: return fallback status
Aggregator->>AggregatedDataVariants: persist fallback status
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Common/ColumnsHashing.h (1)
102-141: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve the lookup hash map here too.
cached_sort_keysis already reserved for the expected distinct count, butraw_to_sort_key_indexstill grows from its default capacity and will rehash repeatedly as the cache fills. Reserving it alongside the vector avoids avoidable rehashes.♻️ Proposed fix
collation_sort_key_cache_active = true; collation_sort_key_cache_distinct_limit = rows / max_collation_sort_key_cache_distinct_ratio; cached_sort_keys.reserve(collation_sort_key_cache_distinct_limit + 1); + raw_to_sort_key_index.reserve(collation_sort_key_cache_distinct_limit + 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Common/ColumnsHashing.h` around lines 102 - 141, Reserve raw_to_sort_key_index for collation_sort_key_cache_distinct_limit when initializing the sort-key cache, alongside the existing cached_sort_keys reservation, so filling the cache does not trigger repeated hash-map rehashes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dbms/src/Common/ColumnsHashing.h`:
- Around line 102-141: Reserve raw_to_sort_key_index for
collation_sort_key_cache_distinct_limit when initializing the sort-key cache,
alongside the existing cached_sort_keys reservation, so filling the cache does
not trigger repeated hash-map rehashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06cf7ea1-eedb-4167-958d-12fb9dbf3da4
📒 Files selected for processing (6)
dbms/src/Common/ColumnsHashing.hdbms/src/Common/ColumnsHashingImpl.hdbms/src/Flash/tests/bench_aggregation_hash_map.cppdbms/src/Flash/tests/gtest_aggregation_executor.cppdbms/src/Interpreters/Aggregator.cppdbms/src/Interpreters/Aggregator.h
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
Pull request overview
This PR improves HashAgg performance for case-insensitive string GROUP BY keys by avoiding repeated collation sort-key generation when raw string values repeat frequently. It introduces a bounded, block-scoped cache in the batch string-key processing path and records a worker-level “once fallback” flag to avoid repeated cache overhead on high-NDV inputs.
Changes:
- Add a collation sort-key cache for batch string-key hashing when using CI collations, with a distinct-key limit to bound overhead.
- Persist a worker-level fallback flag (
AggregatedDataVariants) to skip cache attempts after a high-NDV fallback is detected. - Add unit tests and a micro-benchmark covering semantics, cache sizing behavior, and fallback behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dbms/src/Interpreters/Aggregator.h | Adds worker-level flag to remember collation sort-key cache fallback. |
| dbms/src/Interpreters/Aggregator.cpp | Initializes cache per execution range and propagates fallback to the worker variant. |
| dbms/src/Common/ColumnsHashingImpl.h | Adds no-op cache hooks to the generic hash method base. |
| dbms/src/Common/ColumnsHashing.h | Implements the CI-collation sort-key cache in the batch string-key handler. |
| dbms/src/Flash/tests/gtest_aggregation_executor.cpp | Adds unit tests validating semantics and worker-level once-fallback behavior. |
| dbms/src/Flash/tests/bench_aggregation_hash_map.cpp | Adds benchmarks for low/high NDV and NDV transition workloads for the new cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| agg_process_info.resetBlock(*block); | ||
| do | ||
| { | ||
| aggregator->executeOnBlock(agg_process_info, *data_variants, 1); |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test pull-sanitizer-asan |
1 similar comment
|
/test pull-sanitizer-asan |
windtalker
left a comment
windtalker
left a comment
There was a problem hiding this comment.
Looks like this cache only works for cases that group by a single string column, it does not work for something like group by int_col, string_col?
| const StringRef & raw_key, | ||
| std::string & sort_key_container) | ||
| { | ||
| if (!collation_sort_key_cache_active) |
There was a problem hiding this comment.
add unlikely hint for this if?
| auto * aggregates_pool = result.aggregates_pool; | ||
| typename Method::State state(agg_process_info.key_columns, key_sizes, collators); | ||
| if (!result.string_collation_key_cache_fallback) | ||
| state.initCollationSortKeyCache(agg_process_info.end_row - agg_process_info.start_row); |
There was a problem hiding this comment.
agg_process_info.end_row - agg_process_info.start_row is not always represent a whole block, which means the cache is not block level cache, is it expected?
There was a problem hiding this comment.
Good point. In the normal aggregation path, start_row is 0 and end_row is block.rows(), so the cache is initialized with the whole Block size and is reused across all mini-batches processed by the same executeImplInner() invocation.
The range can be smaller than a whole Block when aggregation is interrupted by a partial-processing path, such as spilling after a ResizeException. In that case, a subsequent executeOnBlock() invocation processes the remaining rows and creates a new local cache for that range.
This is expected. The cache is intentionally scoped to the current processing range rather than guaranteed to span the entire input Block. The cache only affects sort-key computation and does not affect correctness. The worker-level fallback state is kept separately in AggregatedDataVariants::string_collation_key_cache_fallback, so once fallback happens, later Blocks still skip cache attempts.
Yes, this cache currently only applies to |
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: windtalker, xzhangxian1008 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #10979
Problem Summary
For string
GROUP BYkeys using a case-insensitive collation such asutf8mb4_general_ci, HashAgg needs to generate a collation sort key before probing or inserting into the hash table.Previously, the sort key was generated for every input row. When the grouped column has a low number of distinct raw values, the same raw string is converted to the same sort key repeatedly, causing unnecessary collation processing and memory writes.
For example:
l_shipmodeusually has a very low NDV. If it usesutf8mb4_general_ci, repeatedly generating sort keys for millions of rows can consume a significant portion of the HashAgg execution time.At the same time, unconditionally caching sort keys is not suitable for high-NDV inputs because maintaining the cache may introduce additional hash lookups and memory usage without enough cache hits. Therefore, the optimization needs to provide substantial benefits for low-NDV workloads while keeping the high-NDV overhead small and bounded.
What is changed and how it works
This PR adds an always-on collation sort-key cache to the batch string-key processing path of HashAgg.
For each input Block, the cache maps a raw string value to its generated collation sort key:
When processing a row:
The cache is enabled only when all of the following conditions are satisfied:
To bound the cost for high-NDV inputs, the maximum number of cached distinct values is calculated from the number of rows in the current Block:
For a typical maximum runtime Block containing 65,536 rows, the cache can hold at most 2,048 distinct raw values.
If a cache miss occurs after the distinct-value limit has been reached, the current Block falls back to direct sort-key generation. The fallback state is also recorded in the worker's
AggregatedDataVariants, so subsequent Blocks processed by the same aggregation worker do not attempt to initialize the cache again. This worker-level once-fallback behavior prevents high-NDV workloads from repeatedly paying the cache construction and lookup overhead for every Block.The cache itself remains local to one Block. It only stores references to raw strings owned by that Block, so no input-column memory needs to be retained across Blocks. Only the fallback decision is preserved across Blocks.
The optimization does not change collation or grouping semantics. Hash-table keys are still the sort keys generated by the existing collator; the change only avoids regenerating an identical sort key for repeated raw string values.
The benchmark uses 128 Blocks with 65,536 rows per Block:
The results show substantial improvements for low-NDV and mixed workloads, while the worker-level fallback keeps the high-NDV overhead below 1% in the benchmark.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Performance
Bug Fixes
Tests