jacktengg opened a new pull request, #70512:
URL: https://github.com/apache/doris/pull/70512
### Background
Distinct streaming aggregation emits new grouping keys as input arrives. BE
can use this operator for GROUP BY without aggregate functions. A final
aggregate can also contain HAVING and LIMIT.
### What problem does this PR solve?
Issue Number: None
Related PR: None
PR Type: Bugfix
Problem Summary:
A final distinct streaming aggregate can discard valid groups when it has
HAVING and LIMIT. For example, HAVING rejects group 1 and accepts group 2, but
LIMIT 1 stops input after group 1. Apply the final limit after HAVING so the
query returns group 2.
### Details
- **Frequency:** Queries can lose rows when an early group fails HAVING and
consumes the limit before filtering.
- **Root cause:** push() truncated the aggregate block and set _reach_limit
before pull() evaluated HAVING.
- **Why existing tests missed it:** The existing operator tests covered
LIMIT and local preaggregation without a final HAVING filter.
- **Fix:** Skip early truncation when conjuncts are present. Reuse
reached_limit() after the final HAVING filter. Preserve output accounting for
local preaggregation and its handling of rows that bypass deduplication.
- **Other paths and branches:** Ordinary aggregation and streaming
aggregation already filter before applying the final limit. No release branch
is changed.
- **Behavior change:** LIMIT counts only groups that pass HAVING in the
final distinct streaming aggregate.
### Testing
- Before the fix, all four new BE tests failed. The new SQL suite also
failed: it expected 2 and received no row.
- Built ASAN BE with `DISABLE_BE_JAVA_EXTENSIONS=ON DISABLE_BE_CDC_CLIENT=ON
./build.sh --be -j24`. The build and its GLIBC_2.17 symbol check passed.
- Ran `./run-be-ut.sh --run --filter='*DistinctStreamingAgg*' -j24`. All 15
tests passed.
- Ran `./run-regression-test.sh --run -d
query_p0/aggregate,query_p0/set_operations,correctness_p0 -s
distinct_streaming_having_limit,distinct_streaming_repeated_keys,test_distinct_streaming_agg_local_shuffle`.
All three suites passed on the local ASAN BE.
- The new SQL suite uses two rows. It checks one final node for HAVING and
LIMIT, then checks the result with distinct streaming on and off at batch sizes
1 and 1024. The expected output was generated by the regression script through
the ordinary aggregate.
- Direct SQL checks returned 2 for both batch sizes. Profiles confirmed the
distinct streaming implementation.
- clang-format v16, check-format, build structure checks, and clang-tidy
passed for the changed scope. clang-tidy used the BE and UT compile commands
with the compiler wrapper's toolchain paths.
### Release note
Fix missing result rows for GROUP BY queries with HAVING and LIMIT when the
final aggregate uses distinct streaming aggregation.
### Check List (For Author)
- Behavior changed: Yes (see Details)
- Does this need documentation: No
- Pre-submission status: Ready
### AI Disclosure
- Tool: OpenAI Codex
- Generated: Code, tests, comments, and this description
- Human review: Pending author review
### Background
<!-- How the feature or module this PR touches works today, in two to four
plain sentences, for a reviewer who doesn't know it. Link the user doc if there
is one. Leave the problem and the fix to Problem Summary. -->
### What problem does this PR solve?
<!-- Issue Number: "close #N" if this PR resolves the issue, "#N" if it only
relates to it, "None" if there is none. A Feature needs an issue. -->
<!-- Related PR: "#N (relation)" for each PR a reviewer should know about,
such as "#123 (introduced the bug)", separated by commas. Write "None" if no
related PR is known. -->
<!-- PR Type: Bugfix, Performance, Feature, Enhancement, Refactor, Test,
Build/CI/Chore, Revert, Release/Dependency, or Other. -->
Issue Number:
Related PR:
PR Type:
Problem Summary:
<!-- Two to five sentences: the problem or need and who is affected; the
approach in one sentence; when the diff is larger than that suggests, what else
it had to handle, or that most of it is tests; and what existing users see
after an upgrade. -->
### Details
<!--
For a diff of more than about 300 changed lines, start with "- **Change
map:**": how many lines are production code, tests, generated files, and docs,
then the code in parts, each with the problem it handles and its size.
Then answer each field of your PR type on its own line, as "- **Field:**
answer", in this order:
Bugfix: Frequency, Root cause, Why existing tests missed it, Fix, Other
paths and branches
Performance: Why it was slow, Optimization, Results, Applicability and cost
Feature: Scope, Design, Usage, Default behavior, Compatibility
Enhancement: Before and after, Configs and defaults, Impact on existing
users
Refactor: Moves vs. logic changes, Behavior change, Compatibility,
Performance impact, Rollback
Test, adding tests: Scenarios, What existing tests miss, Fails when broken
Test, fixing a test: Failure, Cause, Stability evidence
Build/CI/Chore: Affected, Developer and release impact, License and
third-party, Rollback
Revert: Why not fix forward, Original author notified, Follow-up
Release/Dependency: Versions, Upstream changes, Compatibility, Rollback
Other: Why Other
"None" is a valid answer when it is true. Don't restate the diff.
-->
### Testing
<!-- The tests, suites, or commands that ran, what each shows, and where it
ran (locally or in CI). For a fix, the test that fails before it and passes
after. -->
### Notes for Reviewers
<!-- Optional; delete this section if there is nothing to say. Why each
non-obvious edit was made, what you are unsure about, and where to start
reading if Details has no Change map. -->
### Release note
<!-- One paragraph on what users will notice, or None. -->
### Check List (For Author)
<!-- Behavior changed: "No", or "Yes (see Details)" with a Behavior change
field in Details. -->
<!-- Does this need documentation: "No", or "Yes" with the
apache/doris-website PR or the plan. -->
<!-- Pre-submission status: "Ready"; "Partially verified: " and what is not
verified; or "Not ready: " and why, for a Draft PR. -->
- Behavior changed:
- Does this need documentation:
- Pre-submission status:
### AI Disclosure
<!-- Tool: the agent and model, or None. Generated: the parts it wrote
(code, tests, this description), or None. Human review: what you reviewed of
the generated parts, or "Not applicable" without AI tools. You may add "-
Prompts:" to summarize how you directed the agent. -->
- Tool:
- Generated:
- Human review:
--
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]
