Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
Pull request overview
This PR introduces a performance optimization for hash table operations in aggregation operators by implementing batch prefetching with a two-phase approach and removing an unnecessary conditional check for string hash maps.
Changes:
- Removed conditional
is_string_hash_map()check before prefetching, enabling prefetch for all hash map types - Added
lazy_emplace_batchmethod with two-phase prefetch optimization for better cache utilization - Updated aggregation operators to use batch emplace instead of loop-based single emplace
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| be/src/vec/common/hash_table/hash_map_context.h | Added batch emplace method with two-phase prefetching; removed is_string_hash_map() conditionals from prefetch calls |
| be/src/pipeline/exec/streaming_aggregation_operator.cpp | Updated to use lazy_emplace_batch for both limit and non-limit cases |
| be/src/pipeline/exec/aggregation_source_operator.cpp | Updated to use lazy_emplace_batch instead of loop-based emplace |
| be/src/pipeline/exec/aggregation_sink_operator.cpp | Updated to use lazy_emplace_batch for both limit and non-limit cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (current_row_ref) *current_row_ref = i; | ||
| places[i] = *state.lazy_emplace_key(*hash_table, i, keys[i], hash_values[i], creator, | ||
| creator_for_null_key); | ||
| } |
There was a problem hiding this comment.
Inconsistent code style: line 168 has the conditional and assignment on the same line, while lines 156-158 have them on separate lines. For consistency and readability, please format line 168 the same way as lines 156-158.
| if (current_row_ref) *current_row_ref = i; | |
| places[i] = *state.lazy_emplace_key(*hash_table, i, keys[i], hash_values[i], creator, | |
| creator_for_null_key); | |
| } | |
| if (current_row_ref) | |
| *current_row_ref = i; | |
| places[i] = *state.lazy_emplace_key(*hash_table, i, keys[i], hash_values[i], creator, | |
| creator_for_null_key); |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)