jacktengg opened a new pull request, #69669:
URL: https://github.com/apache/doris/pull/69669
### What problem does this PR solve?
Issue Number: N/A
Related PR: #68980
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.
Resolve the branch-4.2 unit test conflict by adding only this PR's test and
helpers with their required includes, retaining all three existing tests
without importing unrelated master-only tests.
### 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 48;
ASAN build completed and all 4 tests passed, including 16 combinations in the
new test
- Validation: clang-format 16 formatting and check scripts passed for
the two modified C++ files
- Validation: both modified C++ files passed Clang Analyzer with the
toolchain resource directory explicitly supplied
- Full clang-tidy could not complete because of a pre-existing unmatched
NOLINTEND in be/src/core/types.h and the default toolchain resource path
- SQL regression files preserved from the original PR; SQL regression
not rerun for this backport
- Behavior changed: Yes, correct descending NULL placement in ordered
GROUP_CONCAT
- Does this need documentation: No
(cherry picked from commit 3d7479ac946a2419856e824fb316573cc7d39417)
### 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]
