Repository navigation
[improvement](be) Keep one NTILE bucket per function in non-spill windows - #69686
Merged
HappenLee merged 1 commit intoOct 29, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#68500 Problem Summary: The non-spill analytic sink expanded each NTILE literal bucket argument once per input row and retained the resulting column, although NTILE reads only its first value. Store one bucket per function on the first nonempty Block and preserve it when pruning buffered rows. Argument storage now stays constant per NTILE function rather than growing with retained input rows; SQL results remain unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test and Regression test - ASAN BE build and 65 AnalyticSinkOperatorTest cases, including the new multi-Block and pruning case - window_function_spill and test_ntile_function SQL regressions - Behavior changed: No (internal memory use is reduced; SQL results are unchanged) - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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 verified correctness or maintainability finding was identified at b9f59b5. Both changed files and the upstream/downstream NTILE execution path were reviewed. No existing inline findings or additional user focus points were supplied; a fresh PR comment readback was also empty.
Critical checkpoint conclusions:
- Goal and evidence: the non-spill sink copies one positive NTILE bucket per function from the first nonempty Block and preserves it across Blocks and partitions. The new unit test checks two distinct buckets, empty input, a cross-Block partition, pruning, scalar cardinality, and complete output values.
- Scope and clarity: the implementation is limited to NTILE materialization/pruning, one concrete .cpp include, and one focused unit test. Nearby comments explain the scalar and pruning invariants; lint suppressions are narrow and documented.
- Concurrency and locking: the scalar belongs to the sink local state. The source consumes completed output Blocks through existing locks; no new shared mutation, lock ordering, or blocking operation is introduced.
- Lifecycle and memory: insert_from copies the integer into the existing type-matched column storage. Partition resets preserve the bucket; close releases the columns. Existing tracked allocation and reservation paths are retained; no new static initializer or ownership cycle is introduced.
- Configuration: no configuration item or dynamic-update behavior changes.
- Compatibility: no function signature, wire/storage format, or external ABI contract changes; no new libc API is introduced. Release artifacts were not built or audited.
- Thrift/Protobuf: neither directory is changed.
- Parallel paths: non-NTILE arguments retain row-wise materialization/pruning. The separate spill path already retains one bucket per function and is selected before the changed non-spill path.
- Conditional checks and errors: empty Blocks cannot initialize the bucket. FE validation, reinforced during final expression translation, requires a positive nonnullable integer literal; ColumnConst unwrapping preserves its concrete width. Expression execution errors remain propagated with RETURN_IF_ERROR. The NTILE evaluator reads only argument index zero.
- Test coverage: expected NTILE(2)/NTILE(3) outputs and the pruning trigger were checked against the execution logic. Existing FE and SQL tests cover invalid/nonconstant/NULL/unfolded buckets and planner-folding boundaries; existing spill tests cover large buckets and multiple batches.
- Test results: no result files are changed; the new expected values are consistent with partition sizes 4, 2, 3, 3, 3.
- Observability: existing aggregate-computation, memory, and pruning counters/timers cover the affected work; no additional critical instrumentation need was identified.
- Transactions and persistence: no transaction, visibility-version, delete-bitmap, EditLog, replay, or failover behavior changes.
- Data writes and atomicity: this is query-local window evaluation; no write/commit path changes.
- FE-BE variables: no new field or variable must be transmitted.
- Performance: the source establishes bucket storage proportional to the number of NTILE functions, eliminating repeated row expansion and argument buffering. This is a static complexity conclusion, not a measured benchmark.
- Other risks: all initial scalar/type and pruning/consumer concerns were dismissed with concrete producer/consumer evidence. No unresolved source-level candidate remains.
Validation limits: static inspection only; no builds, tests, benchmarks, style/static-analysis tools, or artifact audits were executed, and author-reported runs were not independently reproduced. The added test directly exercises BIGINT buckets and NTILE-only combinations at the BE_TEST pruning threshold; smaller integer widths, mixed row-wise functions, and the production 256-Block threshold were traced statically rather than exercised here.
Convergence: converged after 2 review rounds, including an unchanged-payload final pass by both complete-review agents and the separate scalar/pruning risk reviewer. No new inline comments or existing blocking comment IDs are warranted.
Review converged after 2 round(s).
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29103 ms |
Contributor
TPC-DS: Total hot run time: 151290 ms |
Contributor
ClickBench: Total hot run time: 24.51 s |
Contributor
TPC-H: Total hot run time: 28672 ms |
Contributor
TPC-DS: Total hot run time: 151117 ms |
Contributor
ClickBench: Total hot run time: 24.23 s |
Member
Author
|
run nonConcurrent |
mrhhsg
marked this pull request as ready for review
October 28, 2026 00:54
HappenLee
approved these changes
Oct 29, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the NTILE argument producer/consumer and pruning paths. Retaining one positive literal bucket per function preserves existing results and avoids repeated materialization. No blocking findings. A mixed NTILE/SUM pruning test would be a useful follow-up. Current CI checks are passing; local builds and tests were not rerun as part of this review.
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: None
Related PR: #68500
Problem Summary: The non-spill analytic sink expanded each NTILE literal bucket argument to every input row and appended it to a buffered argument column. NTILE reads only the first value, so the extra materialization and storage grew with the number of retained rows. Store one bucket value per NTILE function from the first nonempty input Block and retain it when old input rows are pruned. Other window functions continue to use their row-wise argument columns. Query results are unchanged.
Release note
None
Check List (For Author)
AnalyticSinkOperatorTest.*(65 tests), including the new multi-Block, multi-partition pruning case.spill_p0/window_function_spillandquery_p0/sql_functions/window_functions/test_ntile_function(one suite each, zero failures).git diff --check.