[fix](exec) Retain sliding window rows during eviction - #67274
Merged
HappenLee merged 2 commits intoAug 31, 2026
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Streaming analytic execution could erase rows that a bounded ROWS frame still needed for the next incremental update. Rebased partition and frame coordinates could then become negative, causing nullable aggregates to access the null map out of bounds or non-nullable aggregates to return incorrect results. Preserve the earliest row required by the next sliding frame before evicting buffered data, and clamp rebased boundary coordinates to the retained columns.
### Release note
Fix potential crashes and incorrect results for bounded sliding ROWS window aggregates when streaming input rows are evicted.
### Check List (For Author)
- Test: Not run; compilation and test execution were deferred for review.
- Added unit coverage for eviction across a bounded ROWS frame and nonnegative boundary rebasing.
- clang-format, diff checks, and BE build hygiene passed.
- Behavior changed: Yes. Sliding ROWS eviction retains rows required by the next incremental frame.
- Does this need documentation: No
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.
Request changes
The bounded sliding ROWS calculation and both changed test expectations are correct, but the fix does not cover the parallel unbounded-start ROWS executor. That path can still delete unread input and return wrong analytic results once streaming reclamation starts; the blocking trace and fix direction are inline.
Critical checkpoints
- Goal and proof: The new guard correctly retains
frame_start - 1for bounded sliding frames, the widened sum test fails the base wrong-result path and proves later safe reclamation, and theBoundaryPosetest correctly proves saturation. The overall reclamation goal is incomplete becauseUNBOUNDED PRECEDING ... N PRECEDINGbypasses the new flag and guard. - Scope, clarity, and parallel conditions: The three-file patch is small and the bounded-frame formula is exact, conservative only for fallback aggregates, and eventually permits deletion. The executor-shape flag is too narrow: the omitted-start PRECEDING executor has the same next-read retention requirement. CURRENT/FOLLOWING, whole-partition, bounded-RANGE, repeated rebase, partition reset, and EOS paths were traced without another in-scope defect.
- Concurrency and lifecycle: Reclamation, frame coordinates, and aggregate state are sink-local and mutate before input append/evaluation. Existing output-buffer and EOS locks are unchanged; no new thread, lock-order, static-initialization, ownership, or teardown issue was found.
- Configuration, compatibility, persistence, and writes: No configuration, thrift/wire symbol, persisted format, transaction, EditLog, data-write, cloud split, or rolling-upgrade behavior changes.
- Tests and results: The modified sums
{0,1,3,6,10,15,20,25,30,35}and pose{0,3}are correct. Existing tests do not exercise the omitted-start PRECEDING reclamation failure. No build, unit test, or regression test was run by this automated review, as required by the review prompt; the pure-text header-hygiene gate passed. - Errors, memory, and nullability: No
Status, exception, allocation, tracker, or error-recovery behavior changes. The bounded retention check is pre-dispatch and type-independent, so an additional nullable reproduction would be useful hardening but is not a separate blocker. - Performance and observability: The new work is constant-time per reclamation attempt with no allocation or rescan. Existing removal counters, timer, and block-memory high-water mark remain sufficient; no new metric or log is required.
Follow-up observations, not blockers for this PR: base and head both retain stale physical row identity for first_value/nth_value (and retained last_value IGNORE NULLS) across in-place prefix erase, and stale peer coordinates for streaming rank/dense_rank. Neither behavior is caused or worsened by this bounded-ROWS patch, so they should be tracked separately rather than attached as blocking inline comments here.
User focus: No additional focus was supplied.
Review status: Complete after three rounds. All Round 3 reviewers returned NO_NEW_VALUABLE_FINDINGS; every candidate was accepted, deduplicated, or dismissed with evidence.
| _executor.get_next_impl = &AnalyticSinkLocalState::_get_next_for_unbounded_rows; | ||
| } else { | ||
| _executor.get_next_impl = &AnalyticSinkLocalState::_get_next_for_sliding_rows; | ||
| _is_sliding_rows = true; |
Contributor
There was a problem hiding this comment.
This flag is not set for ROWS BETWEEN UNBOUNDED PRECEDING AND 5 PRECEDING, so the new retention gate still lets _get_next_for_unbounded_rows() lose its next unread row. With two-row BE_TEST blocks containing 1-8, before row 6 the state contains only row 0/value 1 and the next evaluation must add row 1/value 2, but cutoff 2 erases rows 0-1. After rebasing, the add range clips empty and row 6 returns 1 instead of 3. Production has the same wrong-result path when the PRECEDING offset exceeds the 256-block reclamation distance. Please extend retention to this executor (preserving max(partition_start, current + rows_end_offset)) and add a focused regression.
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: Streaming reclamation for ROWS windows protected bounded sliding frames but could still erase the next unread row required by an UNBOUNDED PRECEDING to N PRECEDING frame. Track the ROWS executor shape and retain the earliest row required by either bounded sliding or unbounded-start evaluation.
### Release note
Fix incorrect results for unbounded-start ROWS window aggregates during streaming input eviction.
### Check List (For Author)
- Test: Not run; compilation and test execution were deferred for review.
- Added unit coverage for unbounded-start ROWS eviction.
- clang-format, diff checks, and BE build hygiene passed.
- Behavior changed: Yes. ROWS eviction retains the next unread input row for unbounded-start frames.
- Does this need documentation: No
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary for current head 9a7fd0cafda68df7fc73f0e9763287c46887a52d
The prior blocking concern is resolved at aggregate HEAD: the eviction path now classifies both streaming ROWS executors, retains the bounded sliding frame's outgoing row, and retains the unbounded-start executor's next unread row. I found no additional blocking issue in the changed code.
Critical checkpoints
- Goal and proof: The strict cutoff is correct for half-open prefix erase. Sliding incremental functions need
frame_start - 1; unbounded-start cumulative evaluation needscurrent + rows_end_offset. The partition-start maximum covers clipped/empty early frames, and positive-FOLLOWING prefill remains protected by the older output-block gate. The new tests select both executors and assert the expected results across the BE_TEST reclamation boundary. - Scope and clarity: The change is focused to executor classification, the constant-time retention gate, nonnegative physical-coordinate rebasing, and targeted BE unit coverage. The three-file scope is appropriate.
- Concurrency and lifecycle: The changed fields and coordinates are sink-local. Existing source-buffer and EOS locking are unchanged, and no new thread, lock-order, static-initialization, ownership, or teardown issue was found.
- Parallel paths and conditions: PRECEDING, CURRENT ROW, FOLLOWING, incremental, reset/recompute, nullable, min/max rescan, partition transition, RANGE, whole-partition, and forced non-streaming paths were traced. Saturation collapses only an erased prefix; live ends and queued boundaries remain valid after rebasing.
- Errors, memory, performance, and observability: No Status/exception path or allocation ownership changes. The gate adds O(1) work and retains only the rows required by the active frame/next unread input until the block-aligned cutoff becomes safe. Existing removal counters/timer and memory high-water mark remain sufficient.
- Configuration, compatibility, persistence, and writes: No configuration, FE-BE variable, function symbol, wire/storage format, EditLog, transaction, data-write, cloud-mode, or rolling-upgrade behavior is changed.
- Tests and standards: The widened sliding sums and new unbounded-start sums are correct, and
BoundaryPosesaturation is covered. No build, unit test, or regression test was run by this automated review, as required by the review prompt. The pure-text BE build-hygiene gate passed. - Candidate disposition: A Round 1 reviewer reproduced stale physical-row identity in pointer/offset-retaining
first_value/nth_valuestate. Base comparison proves the same output-only eviction and in-place erase predate this PR, aggregate HEAD can only defer that eviction, and the live prior review already records it as a separate follow-up. It was therefore dismissed with evidence rather than duplicated inline.
User focus: No additional user-provided focus was supplied.
Review status: Complete after two rounds. All Round 2 normal and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, deduplicated, or dismissed with evidence. No new inline comments are proposed.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16930 ms |
Contributor
TPC-DS: Total hot run time: 82579 ms |
Contributor
ClickBench: Total hot run time: 14.81 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 31, 2026
Problem Summary: Streaming `ROWS` window aggregates retain state across
frame evaluations. The eviction path previously considered only whether
buffered blocks had been emitted, so it could erase either the outgoing
row needed by a bounded sliding frame or the next unread row needed by
an `UNBOUNDED PRECEDING ... N PRECEDING` frame. After rebasing, negative
partition and outgoing positions could allow a nullable aggregate to
access its null map out of bounds. Evicting either kind of required row
could also produce incorrect aggregate results.
Root cause: `_remove_unused_rows()` did not account for the earliest row
required by the next ROWS frame evaluation, and
`BoundaryPose::remove_unused_rows()` allowed retained-column coordinates
to become negative.
This change defers block-aligned eviction when the candidate prefix
contains either `frame_start - 1`, the outgoing row required by a
bounded sliding update, or the next unread row required by an `UNBOUNDED
PRECEDING ... N PRECEDING` frame. It also rebases partition and order
boundaries to nonnegative physical-column coordinates. The BE unit
coverage exercises both ROWS executors across eviction boundaries and
verifies boundary rebasing.
Observed ASAN failure before this change (`output/be/log/be.out`):
```text
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1
#0 doris::AggregateFunctionNullUnaryInlineV2<...>::execute_function_with_incremental(...)
be/src/exprs/aggregate/aggregate_function_null_v2.h:595
#1 doris::AggFnEvaluator::execute_function_with_incremental(...)
be/src/exprs/vectorized_agg_fn.cpp:334
#2 doris::AnalyticSinkLocalState::_execute_for_function<true>(...)
be/src/exec/operator/analytic_sink_operator.cpp:385
#3 doris::AnalyticSinkLocalState::_get_next_for_sliding_rows(...)
be/src/exec/operator/analytic_sink_operator.cpp:203
#4 doris::AnalyticSinkLocalState::_execute_impl(...)
be/src/exec/operator/analytic_sink_operator.cpp:358
#5 doris::AnalyticSinkOperatorX::sink_impl(...)
be/src/exec/operator/analytic_sink_operator.cpp:757
SUMMARY: AddressSanitizer: heap-buffer-overflow in
doris::AggregateFunctionNullUnaryInlineV2<...>::execute_function_with_incremental(...)
```
### 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:
apache/doris-website#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 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
3 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: Streaming
ROWSwindow aggregates retain state across frame evaluations. The eviction path previously considered only whether buffered blocks had been emitted, so it could erase either the outgoing row needed by a bounded sliding frame or the next unread row needed by anUNBOUNDED PRECEDING ... N PRECEDINGframe. After rebasing, negative partition and outgoing positions could allow a nullable aggregate to access its null map out of bounds. Evicting either kind of required row could also produce incorrect aggregate results.Root cause:
_remove_unused_rows()did not account for the earliest row required by the next ROWS frame evaluation, andBoundaryPose::remove_unused_rows()allowed retained-column coordinates to become negative.This change defers block-aligned eviction when the candidate prefix contains either
frame_start - 1, the outgoing row required by a bounded sliding update, or the next unread row required by anUNBOUNDED PRECEDING ... N PRECEDINGframe. It also rebases partition and order boundaries to nonnegative physical-column coordinates. The BE unit coverage exercises both ROWS executors across eviction boundaries and verifies boundary rebasing.Observed ASAN failure before this change (
output/be/log/be.out):Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)