Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: A bounded window can become empty after a nonempty frame. The quantile_union reset only restored the first-input flag while retaining the previous QuantileState, so an empty frame returned the previous frame's quantiles. Clear the stored state on reset while retaining the existing first-input behavior. Unit and SQL regression tests cover empty, null-only, partitioned, and nonempty frames, including all state representations and retained results. ### Release note Fix quantile_union results for empty window frames; quantile_percent now returns NaN instead of values retained from the preceding frame. ### Check List (For Author) - Test: ASAN BE unit tests (46 focused cases); isolated SQL regression (new suite plus two existing suites); ASAN BE+FE build with FE Checkstyle; clang-format, header hygiene, clang-tidy, and diff checks. - Behavior changed: Yes (empty window frames no longer inherit the preceding quantile state). - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: Bounded window frames reset the quantile union accumulator for every output row. The empty-frame correctness fix used QuantileState::clear(), which also shrinks its explicit sample vector and makes ordinary nonempty frames allocate it again on every row. Separate logical sample reset from storage release, use the non-releasing reset for aggregate frames, and retain the existing clear() behavior for callers that need to release storage. Cover repeated explicit frames of sizes 2, 3, and 2048, empty results, capacity reuse, and saved result stability. ### Release note None ### Check List (For Author) - Test: ASAN BE unit tests (47 passed); ASAN BE and FE build; three isolated SQL regression suites; clang-format, format check, build hygiene, and changed-line clang-tidy. - Behavior changed: No (preserves the empty-window correctness fix while avoiding repeated allocations). - Does this need documentation: No.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Member
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.
No new actionable findings in the six-file diff at b78e968c2883bd9dd7c0c1277846cdef9a233bb9. No existing inline findings require carrying forward, and no additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: Reset now clears both the first-input flag and samples. Bounded empty or NULL-only frames therefore publish EMPTY, whose percentiles are NaN. Added unit and SQL tests exercise the trigger.
- Scope and clarity: The change is focused: logical reset, reuse from existing clear, aggregate reset, and relevant tests.
- Concurrency: Mutation remains local to the aggregate state. No new threads, shared mutable function state, locks, or lock-order changes; existing digest COW/reader locking remains intact.
- Lifecycle: Reset releases only the accumulator's digest reference. Published results and inputs retain their own ownership; explicit vectors are independently owned. No static-initialization dependency or ownership cycle is introduced.
- Configuration: No configuration changes.
- Compatibility: Object layout, existing function contracts, intermediate/result types and serialized encodings are unchanged. EMPTY uses the existing representation; no new libc API or dependency is introduced.
- Thrift/protobuf: No definitions changed.
- Parallel paths: Nullable/nonnullable aggregation, bounded ROWS/RANGE, partition boundaries and nested reset forwarding use the corrected state reset. Growing windows retain their existing accumulation behavior; grouped/distributed publication remains compatible.
- Conditions and comments: No production conditional is added. The reset comment and capacity test comment match the implementation and actual bounded-frame caller.
- Test coverage: Inspected initial/trailing empty frames, consecutive resets, SINGLE/EXPLICIT/TDIGEST, subsequent adds, NULL-only input, retained input/results, explicit capacity, multi-value frames and partition reset. No new error behavior requires expected-error tests.
- Expected results: Every added SQL result matches its frame boundaries and explicit ordering. For example, FOLLOWING 1..2 over 10/20/30 produces min/median/max 20/25/30, then 30/30/30, then NaN.
- Observability: This deterministic local-state correction needs no additional metrics or logging.
- Transactions and persistence: No EditLog, failover, visibility-version, delete-bitmap or persistent-storage logic changes.
- Data writes and atomicity: No storage write or transaction behavior changes; accumulator lifetime and cleanup remain RAII-managed.
- FE/BE passing: No new fields or variables; existing function names and QUANTILE_STATE types remain consistent.
- Performance and memory: Logical reset retains explicit capacity; clear retains its storage-release behavior. Previous first-input assignment already replaced digest ownership, so the new reset releases it earlier without introducing another frame traversal or digest construction.
- Other issues and limits: All changed files and relevant caller/consumer paths were reviewed. Both initial ownership and capacity risks were dismissed with concrete evidence, and all three reviewers returned NO_NEW_VALUABLE_FINDINGS. No unresolved code candidate or static coverage gap remains.
Static inspection only: no builds, tests, benchmarks, or runtime/ELF validation were performed. Expected results were checked from SQL semantics; their generation provenance was not independently verified.
Review converged after 1 round(s).
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29140 ms |
Contributor
TPC-DS: Total hot run time: 151293 ms |
Contributor
ClickBench: Total hot run time: 24.27 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
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.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: A bounded ROWS window can become empty after a nonempty frame. The
quantile_unionreset previously restored only the first-input flag and retained the precedingQuantileState. Without a subsequent value to merge, an empty or NULL-only frame returned the preceding frame's quantiles. Reset both the first-input flag and the stored samples before each frame. The logical reset retains explicit sample-vector capacity for reuse, whileclear()preserves its storage-release behavior.Release note
Fix
quantile_unionover empty window frames:quantile_percentnow returns NaN instead of values from the preceding frame.Check List (For Author)