[fix](agg) Align complex aggregate null ordering - #67439
Open
Mryange wants to merge 3 commits into
Open
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 |
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
2 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)