Complete static review of a1dcf05. I found two P2 memory-admission issues in the analytic source replay path, both commented inline. No existing P0/P1 inline finding still applies. The earlier P2 thread about small spill writes is addressed by the current write buffer and is not repeated.

Review checkpoints

• Goal and tests: the eligible path incrementally reduces supported full-partition aggregates, records NTILE/peer metadata, spills input when required, and replays rows in order. Dedicated BE unit cases and a 12-query regression suite cover in-memory, forced-spill, boundaries, nullable values, and peer groups. This review was static: I did not run builds, tests, or a cluster, and the checked-in results are author artifacts rather than independent runtime proof.
• Scope and reuse: the implementation uses the existing spill file and pipeline dependency interfaces. The async result writer declaration adjustment is explained by unity-build interaction. No unrelated change was identified.
• Concurrency and lifecycle: sink and source pipeline tasks hand sealed batches through the shared queue under buffer_mutex; EOS uses sink_eos_lock. I found no additional lock-order or wakeup defect. Writers are sealed before readers open, and close paths release readers and batch references. The first inline finding concerns workload-group admission of a non-revocable in-memory batch; the second concerns simultaneous data and peer-sidecar read allocations.
• Configuration and compatibility: FE forwards the new session option through optional Thrift field 233; BE supplies the 64 MiB default and clamps it like adjacent spill limits. This is a per-query option, with no dynamic process setting or storage-format change. Optional Thrift delivery preserves the older-peer path.
• Parallel paths and conditions: spill-disabled, streaming, and unsupported function/frame cases retain the existing evaluator. Capability wrappers, frame gates, partition boundaries, nullable results, and NTILE's positive constant argument were traced; no separate correctness issue was substantiated.
• Test results and observability: the 12 checked-in outputs match across disabled, enabled, and forced-spill modes. The new profile counters distinguish eligible, spilled, and in-memory partitions. Existing tests do not exercise workload-group admission or measure the peak allocation when both replay files are open; those are the two inline findings.
• Persistence, writes, and performance: the change writes temporary spill files but does not alter table transactions, metadata persistence, or data visibility. Small-block coalescing now flushes on threshold, revoke, and seal, addressing the existing P2 performance thread. No other substantiated performance issue was found.

The supplied focus text is -light and names no additional code path; I still reviewed the full changed-file set. Two complete review rounds converged, and all accepted new findings are inline below.

Static review of PR #68500 at head 088339d. Seven new P2 issues are inline. No P0/P1 existing inline finding still applies; the three existing P2 threads were treated as duplicate fences, and their specific coalescing, in-memory queue reservation, and sidecar reservation cases appear addressed on this head.

Review status: Incomplete convergence. The maximum three full review rounds were completed, but round 3 still produced two distinct findings. Every candidate identified in these rounds was validated, deduplicated or dismissed, and all seven accepted findings are inline; another round was not permitted.

Critical checkpoints:

• Goal and tests: The new sink/store/source path supports partition-wide window reductions, ntile and peer results with in-memory and spilled replay. The inspected unit and regression cases cover those modes, and the 36 result sections are internally consistent across 12 queries and three modes. This review was static only: no build or test ran, and the regression fixture does not assert that physical spilling occurred. The memory-pressure cases in the inline findings remain untested here.
• Scope and conditions: The changed paths form a coherent analytic-spill feature plus aggregate capability, query-option, test and documentation changes. I traced eligibility and frame gates, partition and peer boundaries, nullable result handling, and supported versus legacy analytic paths; no separate row-result error was substantiated.
• Concurrency and lifecycle: Sink and source run in separate pipeline tasks; batch queue accesses use its mutex and dependency wakeups. M1 is a reservation-to-dequeue race despite individually locked accesses. I found no additional lock-order issue. Spill writers close before batches are published; reader, error, cancellation, LIMIT and temporary-file ownership paths were traced without a separate lifecycle finding. No cross-translation-unit static initialization was introduced.
• Memory, writes and performance: M2 and M4-M7 cover record-size admission, write scratch, stale or missing result estimates, and hidden revocable buffers. The earlier small-Block write-amplification thread is already addressed by coalescing. Spill files are local temporary data; this PR does not alter table writes, committed-data visibility, EditLog or transactions.
• Configuration and compatibility: The optional FE-to-BE query option, SQL/load forwarding, BE default and clamp, and fallback for unsupported frames were checked. No rolling-upgrade or dynamic-config defect was substantiated. Spill and memory profile counters provide diagnostic context; no separate observability issue was established.

The supplied -light focus was applied to prioritization while the complete changed-file inventory and the relevant upstream/downstream paths were reviewed.

Review complete at head 389d015ff948a26109e6ab5a02fcef935f55c2cc after two convergence rounds, a final 23-file sweep, and live-thread deduplication. One new P2 is inline below: a stale sink spill-write peak can keep an empty EOS call from publishing its final batch under sustained workload-group pressure. Seven still-applicable P2 admission issues already have inline threads (4226675241, 4226675247, 4226675253, 4226675257, 4226675264, 4226675269, 4226675278), so I have not reposted them. Earlier coalescing and initial queued-batch/sidecar reservation threads (4120354529, 4139868859, 4139868867) are addressed at this head. No existing P0/P1 inline finding remains.

Critical checkpoints:

• Goal and tests: The code adds capability-gated spill for eligible non-streaming window partitions. Static inspection of the BE cases and all 12 regression result groups supports the covered sum/count/avg/min/max, NTILE, peer-function, fallback, and streaming cases; each .out group matches across spill-disabled, in-memory, and forced-spill modes. Those artifacts do not exercise the reported workload-group pause or prove runtime memory bounds.
• Scope and reuse: The changes are focused on analytic sink/source, a separate batch store, aggregate capabilities, one query option, and tests/docs. They reuse the existing SpillFile, pipeline dependency, and aggregate interfaces; no unrelated product path was found in the authoritative diff.
• Concurrency and locks: Sink and source run in separate pipeline tasks. Sealed batches are published and dequeued under buffer_mutex; EOS uses sink_eos_lock without a reverse acquisition path found. The source is signalled after publication, and the sink is signalled when the queue empties. The new P2 concerns admission before that publication, not a lock inversion.
• Lifecycle and static initialization: The sink closes writers before publishing each immutable batch; source completion/close releases readers and batch ownership, and unfinished writer destruction attempts close. No new cross-translation-unit static initialization dependency was found. Failure/cancellation cleanup was inspected statically; no fault-injection run was performed.
• Configuration and FE/BE compatibility: The session variable reaches ordinary SQL, INSERT SELECT, and CTAS through toThrift; BE clamps the optional field and uses the same 64 MiB default when it is absent. Broker-load coordinator defaults also leave query spill disabled. No persisted format or function symbol changed; the optional Thrift addition has a default for mixed versions. This session option is evaluated for each query, so no running-query dynamic refresh is expected.
• Parallel paths and conditions: Spill eligibility is gated by every function's capability and the supported frame. Streaming and unsupported nodes retain the prior path; no-ORDER-BY RANGE CURRENT ROW is treated as a whole-partition frame. Partition, peer, nullable, NTILE, and aggregate reset paths were rechecked without a distinct semantic defect.
• Results and coverage: The checked-in .out data is internally consistent across modes but its generation and correctness against an independent oracle were not verified here. BE tests cover in-memory/disk replay, multi-partition batches, nullable results, peer sidecars, revoke, and slicing. Workload-group admission, fault injection, and large-memory behavior need executed validation; the PR description's test and benchmark claims are author reports.
• Observability and performance: Spill mode, partition, peer metadata, and common spill counters support diagnosis. Coalescing and batch-size slicing address obvious write/read amplification; existing inline threads and the new P2 identify the remaining admission risks. No benchmark was run in this review.
• Persistence and writes: This change writes temporary spill files, not table data or EditLog state; transaction, visible-version, delete-bitmap, and failover checkpoints are not applicable. The temporary-file writer/reader lifecycle and error propagation were reviewed as above. No separate data-visibility issue was substantiated.
• Other issues: No additional substantiated finding remained after deduplication and the final changed-file sweep.

The user focus file contained only -light and named no additional code concern; the complete PR was reviewed. Review work was static only, except the text-only check-build-hygiene.sh gate, which passed. No build, BE unit test, or regression test was run in this invocation.

Reviewed all 27 changed files at af6a803 with two general-scope reviewers and a separate admission/concurrency reviewer. One new P2 is reported inline: a revocation test's input allocation reaches the direct-write threshold, leaving zero revocable bytes and invalidating its expectation. This is a static code-path proof; no build or runtime test was executed.

Four previously reported concerns still apply and are not reposted: retained NTILE parameter accounting, combined coalesced-write admission, the separate first-slice reservation test expectation, and a residual case of initial replay scheduling. The scheduler change covers the sink and later source submissions, but an initial source submission can occur before sink open sets spill_enabled; publication before that source reaches its dependency check can still permit synchronous replay I/O in the simple pool. There are no existing P0/P1 comments to carry forward. The current head addresses the previous adaptive-boundary-search, peer-range-fill, fixed-width nullable-cut, fixture-manager-lifetime and other concrete replay-admission reports.

Critical checkpoint conclusions:

1. Goal and proof: eligible full-partition reductions, NTILE and peer functions use collection/sealed-batch/replay; streaming and unsupported windows retain the existing path. Positive tests support the intended behavior, subject to the inline test defect and existing concerns above.
2. Scope and reuse: changes are focused on analytic spill, shared writer measurements, one query option, tests and design documentation; aggregate update/result and spill-file infrastructure are reused.
3. Concurrency and locks: sink/source publication is protected by buffer_mutex, EOS by sink_eos_lock, and eligibility is atomic. I/O runs outside queue locks; no new lock inversion was verified. Per-task revoke gating prevents simultaneous store mutation. Initial scheduler classification retains the existing concern described above.
4. Lifecycle and initialization: seal closes writers before publication; source owns immutable result descriptors and closes readers before releasing files. Store writers are destroyed before their files; the fixture manager now outlives fixture-owned state. No new ownership cycle or cross-TU initializer dependency was identified. NTILE metadata accounting remains an existing issue.
5. Configuration: the session threshold is forwarded per query, defaults to 64 MiB and is clamped in BE to 1 MiB through 4 GiB; a newly submitted query observes its session value without a process restart.
6. Compatibility: no durable storage or existing spill-record format changes; record/footer maxima are process-local descriptor metadata and existing block serialization remains in use.
7. Thrift/protobuf: read TQueryOptions on HEAD and the current branch-3.1, branch-4.0, branch-4.1 and branch-4.2 refs. New field 237 is optional and unused on those releases; it introduces no field-id collision.
8. Parallel paths: supported capabilities and both nullable adapters are covered; variable-length MIN/MAX, unsupported mixtures and streaming/offset frames fall back. Local and remote spill share the writer/reader implementation; remote coalescing is accounted separately.
9. Conditions: frame selection, complete-key boundary search, spill thresholds and peer-range filling have concrete invariants/comments. Required upstream ordering makes complete-key equality contiguous, including composite and nullable keys.
10. Test coverage: reviewed eligibility, reductions/NULLs, scalar parameters, cross-Block partitions/peers, coalescing, oversize records, revoke, replay slicing and admission cases, plus 15 regression queries in three modes. Runtime fault injection, remote/early-close cleanup, tracked-memory-drop and adversarial numeric/performance validation remain unexecuted or absent from the added tests.
11. Expected results: all 45 regression output sections agree across modes; independent arithmetic checks agree with the displayed results. The inline staged-buffer test mismatch and already-reported replay-cut expectation need correction. Output-generation provenance was not independently established.
12. Observability: mode/reason, partition counts/rows/buffered bytes, peer metadata and replay timing supplement existing spill counters and contextual error logs. Profile counters do not remedy missing MemTracker accounting.
13. Transactions and persistence: no EditLog, commit, visible-version, delete-bitmap or master-failover protocol changes.
14. Data writes and failures: writes are temporary query spill files; failed writes/reads propagate failure and unsuccessful seal prevents publication. Cleanup reuses existing SpillFile ownership/GC; committed table data is unchanged.
15. FE/BE propagation: SessionVariable forwarding/toThrift, the optional query option and RuntimeState consumption are connected; point-query and constant-folding paths do not execute analytic windows.
16. Performance: adaptive key search, range result filling, small-block coalescing and sliced replay were traced; no benchmark or runtime memory bound was measured. Combined-write admission remains the existing concern above.
17. Additional review and user focus: rechecked boundary/NULL/scalar, snapshot/EOS, cancellation/LIMIT and ownership cases. No additional user focus was supplied and no other distinct issue was verified.

Static coverage is complete for this invocation. Runtime behavior, sanitizer results and performance are not certified by this review.

Review converged after 2 round(s).

Static review of PR #68500 at bef288e. One new P2 finding: replay under-admits cuts of arrays with nullable elements. Existing scalar fixed-width sizing is repaired, but the recursive complex-column fallback still uses NULL-eliding serialization size as a bound on copied storage.

Critical checkpoint conclusions:

1. Purpose and evidence: the capability-gated collector/replayer implements the intended supported non-streaming windows; the added tests exercise in-memory and spilled execution. Memory admission remains incomplete as described inline and in the known threads below.
2. Scope and reuse: the change is substantial but focused on analytic spill, shared aggregate capabilities, and its query option; it reuses Block and the existing spill infrastructure.
3. Concurrency and locks: queue publication/dequeue use buffer_mutex, EOS uses sink_eos_lock, and execution mode is atomic. The reviewed buffer-to-EOS lock order is consistent. I/O occurs outside the handoff locks; task execution and revocation exclude concurrent mutation of sink-owned state. No additional race or deadlock was substantiated.
4. Lifecycle and initialization: writers close before immutable batch publication; readers close on source cleanup. File ownership is acyclic, writer/file destruction order is sound, and fixture teardown now keeps SpillDataDir alive. No new cross-translation-unit static initialization dependency was found.
5. Configuration: the forwardable per-query option defaults to 64 MiB and BE clamps it to 1 MiB through 4 GiB. A new query observes its session setting without a process restart.
6. Compatibility: no persistent storage or spill-record format change was found. Older receivers can ignore the new optional query option, and absent values use the BE default. Final ELF/glibc compatibility was not assessed in this static-only review.
7. Thrift/protobuf: complete TQueryOptions definitions were compared with the live branch-3.1, branch-4.0, branch-4.1 and branch-4.2 refs. Optional i64 field 237 is unused on those releases; no new collision was found. Historical mismatches in other fields predate this PR.
8. Parallel paths: disabled, streaming and unsupported nodes retain the existing executor. Eligible in-memory batches and local/remote data/peer files were traced. The source snapshot pins the batch admitted before the later EOF transition; newly published unadmitted batches are deferred.
9. Conditions and invariants: frame gates, full composite-key boundaries, final peer sealing and NTILE positive-literal assumptions have concrete upstream guarantees. I/O errors propagate instead of publishing partial batches.
10. Tests: BE cases cover eligibility, NULL reductions, distinct NTILE constants across partitions/batches, adaptive boundaries, replay slicing, sidecars, revoke and cancellation boundaries. Regression coverage spans 16 query families in three modes. Direct remote analytic spill, fault-injected write/close/read/GC failures and actual tracked-memory decline after revoke remain test gaps.
11. Expected results: all 48 checked-in regression sections were inspected; matching query families have identical outputs across modes. Prior fixture/threshold/reservation-expectation defects are addressed on this head. Output regeneration and passing runtime results were not verified here.
12. Observability: spill counters plus mode/reason, partition counts, peak buffering, peer metadata and replay timing cover the new paths; no new hot per-row logging was found.
13. Transactions and persistence: no EditLog, failover, committed-data visibility, visible-version or delete-bitmap path changes apply.
14. Writes and crash behavior: only disposable query spill files are added; publication follows successful close, and cleanup uses the existing spill infrastructure. No new transactional data-write behavior applies.
15. FE/BE propagation: SessionVariable.toThrift and RuntimeState are connected. Constant-folding and point-query paths do not execute this analytic operator.
16. Performance and memory: adaptive complete-key search, coalesced writes and peer-range fills avoid the previous repeated work; scalar NTILE storage is compact. The new array-cut finding and already-known admission concerns remain; no measured performance claim is made by this review.
17. Other concerns: nullable adapters, decimal/floating accumulation order, bucket counts exceeding partition size, no ORDER BY, multiple partitions per Block and early termination were rechecked without another verified correctness finding.

Existing review context: coalesced-write scratch (4348511978), sink partition-result capacity (4347489392), and the broader constant-argument materialization concern (4348010167) remain duplicate-fenced and are not reposted. The specific NTILE expansion/vector, fixed-width nullable slice, peer-range filling, adaptive boundary search, scheduler classification, fixture teardown and test-expectation examples have been addressed. No existing P0/P1 inline finding was present to carry forward.

User focus: no additional focus was supplied; all 29 changed paths and applicable skill checkpoints were reviewed. The review converged after three rounds, including an independent pass over the corrected frozen payload. The concrete inline trigger explicitly sets minimum_operator_memory_required_kb=4096; FE normally forwards 32000 KiB, whereas 4 MiB is only the BE fallback when the option is absent. No unresolved review candidate remains. Validation is static only: no builds, tests, ASAN execution, remote spill execution or final-binary ABI audit was performed.

Review converged after 3 round(s).

Reviewed commit bef288e685e5d940a92a9ef7a1877a67ea16190a.

The supported scope covers full-partition reductions, NTILE, and percent_rank/cume_dist with sliced replay. Existing discussions retain the P2 edge cases around replay-cut admission, coalesced-write admission, and initial replay scheduling for follow-up.

Validation: GitHub currently reports successful compile, BE UT, FE UT, and P0 regression checks. No additional local build or Doris runtime test was run during this review.