[fix](agg) Align complex aggregate null ordering - #67439
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found two correctness issues in the current head. First, changing the shared complex comparator hint to +1 reverses nested NULL ordering for unary complex min/max (and merge/window paths), conflicting with Doris' documented ARRAY aggregate order; the unchanged plain min(arr) expectation already exposes the inconsistency. Please scope the policy or reconcile the aggregate contract and tests. Second, the id=5 max_by(arr, weight) expected row selects a NULL weight even though the nullable variadic wrapper skips rows with any NULL argument; it should remain [1, null, 3, 4]. The new coverage also does not exercise sliding-window/incremental complex min/max behavior.
|
|
||
| bool change_if_less(const IColumn& column, size_t row_num, Arena& arena) { | ||
| if (!has() || column_data->compare_at(0, row_num, column, -1) == 1) { | ||
| if (!has() || column_data->compare_at(0, row_num, column, 1) == 1) { |
Contributor
There was a problem hiding this comment.
SingleValueDataComplexType is also the state for unary complex min/max, not just the *_by key. With this +1, nested ColumnNullable treats NULL as greatest, so [10,NULL] beats [10,5] for max(arr) and loses for min(arr). That conflicts with Doris' documented ARRAY aggregate order (NULL is smallest) and the unchanged maxmin_array_2 expectation. Please keep the aggregate comparator policy consistent with the documented contract (or scope a different hint to the intended *_by path) and add/update plain, merge, and window coverage.
| 3 [3, 1] [11, 22, 33, 44] | ||
| 4 [3, 1] [10] | ||
| 5 [1, null, 3, 4] [1, 2, 3, 4] | ||
| 5 [11, null, null, 55] [1, 2, 3, 4] |
Contributor
There was a problem hiding this comment.
max_by(arr, weight) still uses the nullable weight column as an INT key. AggregateFunctionNullVariadicInline::add drops any row where an argument is NULL, so id=5's [11, null, null, 55] row (weight=NULL) cannot be selected; the maximum eligible weight is 5, yielding [1, null, 3, 4]. Please restore this expected value and retain the NULL-key case to verify it stays skipped.
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: Array NULL ordering changes also affect associated values selected by complex MIN_BY and MAX_BY regression queries. Update the remaining expected results to match the ordering keys exercised by each query.
### Release note
None
### Check List (For Author)
- Test: Regression test
- test_aggregate_all_functions2
- Behavior changed: No
- Does this need documentation: No
Contributor
TPC-H: Total hot run time: 17055 ms |
Contributor
TPC-DS: Total hot run time: 82000 ms |
Contributor
ClickBench: Total hot run time: 14.62 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16750 ms |
Contributor
TPC-DS: Total hot run time: 81398 ms |
Contributor
ClickBench: Total hot run time: 14.74 s |
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16797 ms |
Contributor
TPC-DS: Total hot run time: 80938 ms |
Contributor
ClickBench: Total hot run time: 14.64 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16710 ms |
Contributor
TPC-DS: Total hot run time: 81379 ms |
Contributor
ClickBench: Total hot run time: 14.55 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: the homogeneous exact-head behavior is internally consistent, but the ordering change silently accepts semantically incompatible old partial and persisted aggregate states. I left one inline P1 comment with a concrete information-loss scenario.
Review status: complete for b46c17aad34cac4b4f5043127cbf7c9151854e3d after two rounds. Both normal Round 2 reviewers and the dedicated risk reviewer returned NO_NEW_VALUABLE_FINDINGS; every candidate was accepted, deduplicated, or dismissed.
Critical checkpoint conclusions:
- Goal and correctness: hint
1makes homogeneous complexmin/maxand ARRAY-keymin_by/max_byuse the same nested-NULL-greatest policy as generic complex comparison operators. The direct, batch, merge, and window paths agree within one build, but the PR does not fully accomplish the goal across old partial/persisted states because both old and new builds identify the policy as execution version 13. - Scope and clarity: the source change is small and focused. The missing compatibility boundary is the only accepted code issue.
- Concurrency: no shared mutable state, locks, atomics, or new thread entry points are introduced. Distributed partial aggregation is relevant to the compatibility issue, not to thread safety.
- Lifecycle and persistence: ownership/reset/serialization layout of the one-value complex state is unchanged. Persisted
*_statevalues retain only the old winner, and the absent function-version mapping causes them to be admitted under the new policy; this is the blocking issue. - Configuration and FE/BE propagation: no configuration or transmitted variable is added. The existing FE-supplied BE execution version remains 13 in both base and head, so it cannot select a consistent old/new policy during rolling operation.
- Parallel and special paths: unary ARRAY/MAP/STRUCT
min/max, ARRAY-key*_by, add, optimized batch, merge, serialization, and incremental-window equality were traced. Outer NULL arguments are correctly skipped by the nullable wrapper; NULL elements inside a non-NULL array are correctly compared. Numeric NaN ordering is unchanged. - Error, memory, and compatibility handling: no ignored status, new exception boundary, allocation, COW mutation, or memory-accounting issue was found. Compatibility handling is insufficient as described inline.
- Tests and results: every changed exact-head expected result was independently derived and is deterministic, including id=5
max_by(arr, weight) = [1, null, 3, 4]. P0 regression, BE UT, FE UT, formatter, and other relevant checks are green CI evidence. Per the review prompt, I ran no build or test, so this review is static plus CI evidence; mixed-version/persisted-state coverage is still required for the accepted issue. - Observability, transactions, and data writes: no new logging/metrics need was identified and ordinary transaction visibility is untouched. The only persistence impact is aggregate-state semantic compatibility.
- Performance: the comparator constant does not add copies, scans, allocations, or lock work; no new performance issue was found.
- Repository standards: the PR title is conforming and code-format checks are green. The intermediate
upd casecommit and unchecked PR checklist should be cleaned up before merge, but they are process metadata rather than an additional source defect.
Existing review context and user focus: no additional user focus was provided. The exact head resolves the historical NULL-key expected-output thread. The earlier line-619 contract concern was not duplicated: the code and linked documentation update both define nested ARRAY NULL elements as greater, while the new compatibility finding is a distinct mixed-version/persisted-state failure at the state-merge boundary.
|
|
||
| bool change_if_less(const Self& to, Arena& arena) { | ||
| if (to.has() && (!has() || column_data->compare_at(0, 0, *to.column_data, -1) == 1)) { | ||
| if (to.has() && (!has() || column_data->compare_at(0, 0, *to.column_data, 1) == 1)) { |
Contributor
There was a problem hiding this comment.
[P1] Version the semantic change before merging old aggregate states
This overload merges an already-reduced complex state, but the PR reverses the order used to choose that sole state without creating a compatibility boundary. Both the base and head still advertise BE exec version 13. For example, an old v13 worker computing max over [1, NULL] and [1, 5] serializes only [1, 5]; under this PR v13 max should return [1, NULL], and a new BE merging the old state cannot recover the discarded row. Stored max_state/min_state values (and ARRAY-key *_by states) are likewise accepted because these functions have no compatibility-map entry. Please version/select the old policy for old states, or reject and require rebuilding them, and cover mixed/persisted merges.
mrhhsg
approved these changes
Sep 15, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions Bot
pushed a commit
that referenced
this pull request
Sep 23, 2026
Problem Summary: Complex-type `min`, `max`, `min_by`, and `max_by` used a NULL ordering direction different from ordinary SQL array comparisons. This could select inconsistent results for arrays containing NULL elements. The aggregate comparison direction is aligned with SQL array ordering, and regression expectations are updated. apache/doris-website#4121 ### Release note Complex-type aggregate ordering now follows SQL array NULL ordering. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
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.
Problem Summary: Complex-type
min,max,min_by, andmax_byused a NULL ordering direction different from ordinary SQL array comparisons. This could select inconsistent results for arrays containing NULL elements. The aggregate comparison direction is aligned with SQL array ordering, and regression expectations are updated.apache/doris-website#4121
Release note
Complex-type aggregate ordering now follows SQL array NULL ordering.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)