[fix](be) Fix nth_value for upper bounded windows#64864
Merged
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nereids reverses order keys and window frames for frames ending with UNBOUNDED FOLLOWING so BE can execute an equivalent supported frame shape. nth_value depends on the position within the frame, so reversing the order without adjusting its offset changes the result. This change lets BE interpret negative nth_value offsets as counting from the end of the frame, and rewrites nth_value's offset to 0 - offset when Nereids applies the reverse-window transformation.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Ran git diff --check for the modified BE and FE files. Did not compile per user request.
- Behavior changed: Yes. nth_value keeps equivalent semantics when the window frame is reversed for execution.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoints:
- Goal/test proof: the PR targets nth_value correctness for reversed upper-bounded ROWS frames. The production FE/BE direction looks coherent, and the regression case checks the SQL behavior, but the new BE unit test does not prove the intended executor path because it feeds the aggregate state with a different range protocol.
- Scope/focus: the implementation is narrowly scoped to nth_value offset reversal and BE negative-offset interpretation.
- Concurrency/lifecycle/config/compatibility: no new concurrency, lifecycle-managed resource, dynamic config, storage format, or FE-BE protocol compatibility issue found in the changed production paths.
- Parallel paths: Nereids reversal, physical window translation, BE argument materialization, and ROWS execution were checked. No additional optimizer/rewrite issue found.
- Tests/style: regression output is deterministic and matches the SQL expectation. The accepted issue is in the new BE UT expectations/call sequence. Targeted patch sweep found no extra style candidate in the five live PR files.
User focus: no additional user-provided review focus.
Subagent conclusions: optimizer-rewrite reported no new candidates in the convergence round. tests-session-config reported TSC-001 as a duplicate of MAIN-001, and no new candidate in the convergence round. The final accepted inline comment set contains MAIN-001 only.
| &could_use_previous_result); | ||
| function->insert_result_into(place, *result_column); | ||
|
|
||
| function->add_range_single_place(0, 3, 0, 2, place, columns, arena, &use_null_result, |
Contributor
There was a problem hiding this comment.
The direct test does not mirror the executor protocol for this window shape. After FE reverses ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING, _get_next_for_unbounded_rows() feeds nth_value only the newly added range (current_row_end - 1, current_row_end) on each row. This test instead calls [0,1), then [0,2), then [0,3). With the implementation above, _frame_total_rows becomes 1, then 3, then 6, so offset -2 reads position 1 ("B") for the second result and later goes past the three-row input, rather than producing the expected "C"/"B". Please change the test to feed the same delta ranges as the analytic executor (for example [0,1), [1,2), [2,3)) or reset state between full-frame evaluations; otherwise the added BE UT will fail when it is run.
Contributor
TPC-H: Total hot run time: 28992 ms |
Contributor
TPC-H: Total hot run time: 28858 ms |
Contributor
TPC-DS: Total hot run time: 171739 ms |
Contributor
ClickBench: Total hot run time: 25.24 s |
Contributor
TPC-DS: Total hot run time: 170958 ms |
Contributor
ClickBench: Total hot run time: 25.09 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: nth_value over an upper-bounded/lower-unbounded window frame such as ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING is normalized by reversing the order and frame, then evaluating nth_value with a negative offset over a cumulative frame. The BE nth_value window state replaced the tracked frame row count on each range update, so later rows in the cumulative frame could address the wrong row or return NULL. The fix keeps the cumulative frame row count across range updates, and FE preserves literal offset typing when negating bigint nth_value arguments. This adds BE unit coverage for the reversed cumulative execution path and a regression case comparing nth_value to lead for the original SQL frame.
### Release note
Fix nth_value results for ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING window frames.
### Check List (For Author)
- Test: Regression test; style check
- Regression test: ./run-regression-test.sh --run -d query_p0/sql_functions/window_functions -s test_nthvalue_function -forceGenOut
- Style check: build-support/check-format.sh
- Unit Test: BE UT added but not completed locally because the ASAN UT build was too slow in this environment
- Behavior changed: Yes. Fixes incorrect nth_value results for upper-bounded/lower-unbounded window frames.
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoints:
- Goal/test proof: the PR targets
nth_valuecorrectness for reversed upper-bounded ROWS frames. TheCURRENT ROW AND UNBOUNDED FOLLOWINGregression and updated BE UT now cover that executor protocol, butn FOLLOWING AND UNBOUNDED FOLLOWINGremains incorrect. - Scope/focus: the production change is narrowly scoped to Nereids
nth_valueoffset reversal and BE negative-offset interpretation. - Concurrency/lifecycle/config/compatibility: no new concurrency, lifecycle-managed resource, dynamic config, storage format, or FE-BE protocol compatibility issue found.
- Parallel paths: I checked Nereids frame reversal, BE unbounded/sliding ROWS execution, and the current regression/UT paths. The remaining issue is in the parallel reversed frame shape whose right boundary becomes
n PRECEDING. - Tests/style:
git diff --checkpassed on the exact PR range. I did not run FE/BE/regression tests locally because this checkout is not worktree-initialized andthirdparty/installedis absent.
User focus: no additional user-provided review focus.
Subagent conclusions: optimizer-rewrite proposed OR-001, which was verified and merged into MAIN-001 as the accepted inline comment. tests-session-config reported no new candidates. In final convergence round 1, both optimizer-rewrite and tests-session-config replied NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set after MAIN-001 was accepted.
Contributor
TPC-H: Total hot run time: 29229 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 172327 ms |
Contributor
ClickBench: Total hot run time: 25.41 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
starocean999
approved these changes
Jun 26, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions Bot
pushed a commit
that referenced
this pull request
Jun 26, 2026
### What problem does this PR solve? Related PR: #50559 Problem Summary: nth_value over an upper-bounded/lower-unbounded window frame such as ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING is normalized by reversing the order and frame, then evaluating nth_value with a negative offset over a cumulative frame. The BE nth_value window state replaced the tracked frame row count on each range update, so later rows in the cumulative frame could address the wrong row or return NULL. The fix keeps the cumulative frame row count across range updates, and FE preserves literal offset typing when negating bigint nth_value arguments. This adds BE unit coverage for the reversed cumulative execution path and a regression case comparing nth_value to lead for the original SQL frame. ### Release note Fix nth_value results for ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING window frames.
yiguolei
pushed a commit
that referenced
this pull request
Jun 26, 2026
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
5 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.
What problem does this PR solve?
Related PR: #50559
Problem Summary: nth_value over an upper-bounded/lower-unbounded window frame such as ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING is normalized by reversing the order and frame, then evaluating nth_value with a negative offset over a cumulative frame. The BE nth_value window state replaced the tracked frame row count on each range update, so later rows in the cumulative frame could address the wrong row or return NULL. The fix keeps the cumulative frame row count across range updates, and FE preserves literal offset typing when negating bigint nth_value arguments. This adds BE unit coverage for the reversed cumulative execution path and a regression case comparing nth_value to lead for the original SQL frame.
Release note
Fix nth_value results for ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING window frames.
Check List (For Author)
./run-regression-test.sh --run -d query_p0/sql_functions/window_functions -s test_nthvalue_function -forceGenOutbuild-support/check-format.sh