jacktengg opened a new pull request, #68980:
URL: https://github.com/apache/doris/pull/68980
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: GROUP_CONCAT reversed NULLS FIRST and NULLS LAST when an
ORDER BY key was descending, for both ordinary and DISTINCT aggregation in
one-stage and two-stage plans. With values ('a', NULL) and ('b', 1), DESC NULLS
LAST returned a|b instead of b|a, while DESC NULLS FIRST returned b|a instead
of a|b. AggFnEvaluator constructed nulls_direction without accounting for the
sort direction, which the comparator multiplies into its result. Construct the
null comparison direction with the same rule as the general sorter. Add BE
coverage for all four direction/NULL placement combinations, with and without
DISTINCT and serialized merges, and regression coverage for both aggregation
phases, grouping, duplicate values, multiple sort keys, all-NULL keys, and the
default separator.
### Release note
GROUP_CONCAT now respects NULLS FIRST and NULLS LAST for descending ORDER BY
keys, including DISTINCT and two-stage aggregation.
### Check List (For Author)
- Test:
- Unit Test: run-be-ut.sh --run --filter=AggFnEvaluatorTest.* -j 28; all
25 tests passed, including 16 combinations in the new test
- Regression test: run-regression-test.sh --run -d query_p0/aggregate -s
test_group_concat_null_order,agg_group_concat; both suites passed
- Manual test: verified the original two-row reproduction for ordinary
and DISTINCT aggregation with agg_phase=1 and agg_phase=2
- Validation: ASAN build.sh --be -j 16, clang-format 16, clang-tidy, and
build hygiene checks passed; regression output was generated by the test script
and independently checked
- Behavior changed: Yes, correct descending NULL placement in ordered
GROUP_CONCAT
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### 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:
https://github.com/apache/doris-website/pull/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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
