Uh oh!
There was an error while loading. Please reload this page.
[fix](be) Handle zero-argument count aggregate states - #67679
Open
HappenLee wants to merge 1 commit into
Open
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#58031 Problem Summary: `count_union(count_state())` can crash the BE while initializing an aggregate evaluator. FE represents zero-argument count as `count(*)` and sends an AggState with an empty subtype list, but `DataTypeAggState` unconditionally reads the first subtype to infer the nested result type. Handle zero-argument count with its explicit Int64 result type while preserving the empty argument signature and reusing the existing count implementation. Reject unsupported zero-argument AggState functions with `INVALID_ARGUMENT` before invoking creators that require an input type. Parameterized aggregate handling and serialization formats remain unchanged. ### Release note Fix a BE crash when constructing zero-argument count aggregate states, including queries using `count_union(count_state())` and `count_merge(count_state())`. ### Check List (For Author) - Test: - [x] Unit Test: 20 ASAN tests passed (`DataTypeAggStateZeroArgumentTest.*`, `Params/DataTypeAggStateTest.*`, `AggregateFunctionCountTest.*`). New coverage includes empty/one/three/8193-row state serialization and merge, plus invalid empty-argument functions. - [x] Regression test: `test_count_state_zero_arguments` passed on an isolated ASAN BE cluster. Generated expected output with `-forceGenOut`, then passed a normal comparison run. Covers the original crash, empty input, nullable input, 10001 rows, grouped union, and parameterized Decimal aggregates. BE remained alive afterward. - BE and FE build passed with `./build.sh --be --fe -j 48`; clang-format 16 and build hygiene checks passed. - clang-tidy reported no diagnostics on changed lines. The script could not pass because the base branch has an unmatched `NOLINTEND` in `be/src/core/types.h`; other emitted diagnostics also refer to unchanged code. - Behavior changed: - [x] Yes. Zero-argument count AggState construction succeeds instead of crashing; unsupported empty-argument states raise an error. - Does this need documentation? - [x] No.
hello-stephen
commented
Sep 8, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
HappenLee
commented
Sep 8, 2026
ContributorAuthor
/review |
HappenLee
commented
Sep 8, 2026
ContributorAuthor
run buildall |
Contributor
There was a problem hiding this comment.
Review opinion: LGTM; no blocking issue found.
- Goal and correctness: The change fixes the exact BE failure by handling the only valid zero-argument aggregate-state signature, canonical
count, before indexing its subtype list. The explicitInt64result agrees with FECount, BEAggregateFunctionCount, and merge output. Invalid empty signatures now fail withINVALID_ARGUMENT. - Scope and parallel paths: The production edit is limited to constructor inference. I traced Thrift, protobuf, segment, and tablet-schema reconstruction through scalar
count_state(), constant materialization, union, merge, empty input, grouped aggregation, and multiple input blocks. Nonempty, nullable, and Decimal aggregate paths remain unchanged. - Concurrency and lifecycle: No new shared mutable state, thread entry, lock, lock order, static-initialization dependency, ownership cycle, or special cleanup path is introduced. Aggregate state remains arena-owned and the function factory remains protected by its existing
call_onceinitialization. - Compatibility and persistence: No FE-BE field, storage layout, serialized payload, function symbol, configuration item, EditLog path, transaction path, or data-write protocol changes. The existing BE execution version is preserved in every reconstruction path, and count continues to use the existing fixed-length state payload.
- Tests and result oracles: The BE test covers 0, 1, 3, and 8193 states through serialization/deserialization and merge, plus invalid empty signatures. The regression suite exercises the original query path, empty input/union, nullable and constant arguments, 10001 rows, grouped union, and parameterized Decimal aggregates; every checked scalar result in the generated output is consistent. Per the review-runner instructions I did not build or run tests, but the read-only header-hygiene gate passed all checks.
- Performance, memory, and observability: The new branch is construction-only, uses the existing tracked test buffer and state allocation paths, and adds no hot-path scan or redundant per-row work. The explicit invalid-argument exception is sufficient for the new failure mode; no new metric or log is warranted.
- User focus: No additional user-provided review focus was supplied.
Non-blocking PR-description note: Related PR: #58031 currently points to an unrelated session-variable persistence change; please correct or remove that link if it was accidental.
hello-stephen
commented
Sep 8, 2026
Contributor
TPC-H: Total hot run time: 16724 ms |
hello-stephen
commented
Sep 8, 2026
Contributor
TPC-DS: Total hot run time: 82183 ms |
hello-stephen
commented
Sep 8, 2026
Contributor
ClickBench: Total hot run time: 14.89 s |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Related PR: #58031
Problem Summary:
count_union(count_state())can crash the BE while initializing an aggregate evaluator. FE represents zero-argument count ascount(*)and sends an AggState with an empty subtype list, butDataTypeAggStateunconditionally reads the first subtype to infer the nested result type.Handle zero-argument count with its explicit Int64 result type while preserving the empty argument signature and reusing the existing count implementation. Reject unsupported zero-argument AggState functions with
INVALID_ARGUMENTbefore invoking creators that require an input type. Parameterized aggregate handling and serialization formats remain unchanged.Release note
Fix a BE crash when constructing zero-argument count aggregate states, including queries using
count_union(count_state())andcount_merge(count_state()).Check List (For Author)
DataTypeAggStateZeroArgumentTest.*,Params/DataTypeAggStateTest.*,AggregateFunctionCountTest.*). New coverage includes empty/one/three/8193-row state serialization and merge, plus invalid empty-argument functions.test_count_state_zero_argumentspassed on an isolated ASAN BE cluster. Generated expected output with-forceGenOut, then passed a normal comparison run. Covers the original crash, empty input, nullable input, 10001 rows, grouped union, and parameterized Decimal aggregates. BE remained alive afterward../build.sh --be --fe -j 48; clang-format 16 and build hygiene checks passed.NOLINTENDinbe/src/core/types.h; other emitted diagnostics also refer to unchanged code.Check List (For Reviewer who merge this PR)