[improvement](be) Optimize numeric DISTINCT state merging - #68350
Merged
mrhhsg merged 1 commit intoSep 22, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: Generic numeric DISTINCT aggregates copy the source hash set when merging materialized states. Serialized-state merging additionally populates a temporary hash set. Insert source keys directly and deserialize numeric keys into the destination, reserving only for an empty destination. Keep generic StringRef deserialization and arena ownership unchanged. A standalone phmap merge-kernel microbenchmark with 131072 distinct int64 keys, Clang 21.1.8 and AVX2 measured empty-destination serialized merging at 9.725 ms before versus 0.944 ms after (nine-run medians). These are not end-to-end Doris SQL measurements. ### Release note Reduce temporary hash-set allocations and CPU work when merging generic numeric DISTINCT aggregate states. ### Check List (For Author) - Test: 16 ASAN BE unit tests passed; numeric DISTINCT SQL regression generated and verified; FE/BE build, header hygiene, clang-format 16, and clang-tidy checks passed. - Behavior changed: No; SQL results and serialized formats are unchanged. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review Please focus on numeric DISTINCT merge correctness, source-state preservation, direct deserialization dispatch through grouped/selected/ungrouped and Nullable paths, generic StringRef arena ownership, and empty-only reserve behavior for overlapping sets. Local validation passed: 16 ASAN unit tests, SQL regression generation and verification, FE/BE build, header hygiene, clang-format 16, and clang-tidy. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review conclusion: approval. I found no blocking correctness issue or valuable inline finding.
Critical checkpoints:
- Goal, scope, and tests: the focused change removes the numeric DISTINCT source-container copy and serialized scratch-set population while retaining set semantics. The typed unit tests cover all five integer widths, source preservation, repeat and empty cases, grouped and selected dispatch, both Nullable implementations, and generic string ownership. The regression covers aggregate phases 1 and 2, grouped and ungrouped queries, nullable and non-null inputs, empty and all-null inputs, and AggState merging; the expected arithmetic and NULL results are consistent with the input data.
- Concurrency and lifecycle: aggregate places and arenas remain operator-local; no new shared mutable state, locks, threads, static initialization, or cross-translation-unit lifetime dependency is introduced. Existing helpers still create and destroy scratch states on success and exception. Generic StringRefs are borrowed only in the scratch state and copied into the destination arena before the reader or scratch state is released.
- Compatibility and parallel paths: the serialized representation is unchanged. I traced ordinary grouped, selected, ungrouped/range, state-combine/input state-union, legacy Nullable, and Nullable-v2 paths. Numeric payloads consume the same count and fixed-width values directly into the destination; generic single- and multi-argument states retain deserialize-then-arena-copy behavior.
- Conditions and performance: reserving only for an empty destination is justified because only then is the incoming cardinality the exact final size; nonempty sets may overlap. Direct const iteration preserves the source and avoids the prior full copy, while repeated overlap does not force speculative growth. No obvious hot-path regression remains.
- Configuration, persistence, transactions, FE/BE protocol, data-write atomicity, and observability: not applicable; this PR adds none of these surfaces and does not require new logging or metrics.
User focus: numeric merge correctness, source preservation, all requested deserialization dispatch variants, generic StringRef ownership, and overlap-aware reserve behavior were all explicitly checked; I found no additional issue in those areas.
Validation note: this was a static review as required by the review runner. I did not rerun builds or tests. The PR author reports 16 ASAN unit tests, SQL regression generation and verification, FE/BE build, header hygiene, clang-format 16, and clang-tidy.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28395 ms |
Contributor
TPC-DS: Total hot run time: 153102 ms |
Contributor
ClickBench: Total hot run time: 23.94 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
mrhhsg
approved these changes
Sep 22, 2026
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
3 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.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
The generic numeric DISTINCT combinator (for example,
multi_distinct_sumover integer columns) copies the entire source hash set before merging it. When merging serialized partial aggregates, the default implementation first builds a temporary hash set and then copies that set again. This adds allocation, hashing, and traversal work in the aggregation merge stage.Insert existing source keys directly without modifying the source. Override
deserialize_and_mergeso numeric keys are read directly into the destination set. Reserve the known number of incoming keys only when the destination is empty: reserving the sum of source and destination sizes can unnecessarily grow heavily overlapping sets. Generic/StringRef states keep their existing deserialize-then-merge path, including copying key bytes into the destination arena. Serialization and NULL semantics are unchanged.An earlier standalone merge-kernel microbenchmark with 131,072 distinct int64 keys (Clang 21.1.8, AVX2, nine-run medians) measured serialized merging into an empty destination at 9.725 ms before versus 0.944 ms with direct deserialization, and full overlap at 6.233 ms versus 0.250 ms. This uses phmap with a counting standard allocator, not a full Doris SQL query; these numbers are not end-to-end latency claims. The dedicated
multi_distinct_countimplementation already has its own merge optimization and is outside this change.Release note
Reduce temporary hash-set allocations and CPU work when merging generic numeric DISTINCT aggregate states.
Check List (For Author)
Validation:
./run-be-ut.sh -j 96 --run --filter='DistinctNumericMergeTest/*.*:NullImplementations/DistinctMergeDispatchTest.*': 16 tests passed.build-support/run-clang-tidy.sh: no findings on changed code (header analyzed with a compile command derived from the new test TU)../run-regression-test.sh --run -d query_p0/aggregate -s test_numeric_distinct_merge -genOutgenerated the expected output; rerunning without-genOutpassed all 10 result checks and the merge-plan assertion../build.sh --be --fe -j 96completed with ASAN BE and FE Checkstyle enabled.