[sunchao]

Which issue does this PR close?

Related to #24937. This fixes RANGE-window comparisons; general MIN/MAX scalar comparisons remain separate.

Rationale for this change

RANGE windows can return incorrect counts for ordinary lists containing nulls:

SELECT count(*) OVER (ORDER BY l NULLS FIRST)
FROM (VALUES ([NULL::int]), ([1]), ([NULL::int])) t(l);

The result should be 2,2,3, but the old peer search returns 3,3,3. Lists containing lists or structs can also fail with Uncomparable values, including during streaming execution in Linear mode.

What changes are included in this PR?

Use Arrow's logical comparison to find nested CURRENT ROW peer boundaries. Reuse the comparator while the ordering arrays are identical, release old array ownership when buffered rows are pruned, and preserve WindowFrameStateRange's existing unwind-safety traits. Scalar equality is insufficient for nested dictionary values with different encodings of logical nulls.

Use Arrow comparison for Linear-mode completion as well. Compute its Boolean result lazily once per partition evaluation, avoiding repeated comparator construction inside the row loop. This adds no persistent state or public API.

SQL type admission and finite-offset arithmetic retain their existing behavior.

What is the testing strategy for this PR?

SQL regressions cover the ordinary-list wrong result and the previously failing nested-list queries. The existing core_integration target covers all three nested list/struct fixtures, batch boundaries, and Sorted/Linear execution (the Linear frames end at CURRENT ROW), including completed peers emitted before EOF when a newer row arrives in another partition. State tests cover nested dictionary logical nulls, comparator reuse, ownership release, and unwind-trait compatibility.

Local validation of this update:

• Extended workspace suite: 12,745 Rust tests passed, eight ignored, and all 536 SQL-logic files completed. The final physical test variants also passed a focused rerun.
• CI-configured Clippy, formatting, license/typo checks, generated docs, local links, dependency checks, and Rust/website documentation builds passed.
• Existing bounded-window benchmarks passed smoke checks on upstream base 63af08da and this patch. A focused debug-build dictionary-null probe verifies the removal of repeated comparator setup; no production-throughput measurements are claimed.
• All-target/all-feature Clippy still reports three mutable_key_type errors in unchanged push_down_filter.rs, reproduced on the clean upstream base. A run allowing only that known lint passes with all other warnings denied.
• The security audit reports RUSTSEC-2026-0382 and RUSTSEC-2026-0383 in the unchanged lockfile, also reproduced on the upstream base. Current upstream main already contains the fixed rustls-webpki 0.103.16 and webpki-root-certs 1.0.10 versions.

Are there any user-facing changes?

Already-admitted SQL RANGE windows over nested keys return correct results instead of wrong peer counts or an internal comparison error. Physical window execution also handles these peer boundaries, including Linear-mode completion.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 87.25490% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.92%. Comparing base (63af08d) to head (3ee0478).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/expr/src/window_state.rs	87.25%	6 Missing and 7 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #26067    +/-   ##
========================================
  Coverage   82.91%   82.92%            
========================================
  Files        1155     1155            
  Lines      458058   458304   +246     
  Branches   458058   458304   +246     
========================================
+ Hits       379813   380038   +225     
- Misses      55202    55216    +14     
- Partials    23043    23050     +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sunchao]

cc @viirya @alamb @comphead for review

[viirya]

Thanks for this. Using Arrow's comparator to find CURRENT ROW peers by equality is more robust than the ordering-based scan, since it no longer relies on ScalarValue ordering matching the sort. Two larger points, plus details inline:

Linear / PartiallySorted mode still fails for nested keys. is_row_ahead in window_expr.rs (outside this diff) still calls compare_rows on order_bys[0]. BoundedWindowAggExec reaches it for RANGE ... CURRENT ROW frames whenever it is in linear search mode. I took nested_batches() from this PR, added a constant p column, and ran PARTITION BY p with InputOrderMode::Linear. All three key types fail with Internal("Uncomparable values: List([[]]), List([[]])"). So "the same boundary fix applies to windows constructed through the physical API" currently holds only for Sorted. is_row_ahead needs an ordering rather than equality, so make_comparator(old_col, current_col, opts) over the two arrays looks like a natural fit. Could this be covered here?

This also fixes a wrong result for plain lists, which seems worth claiming and testing. On main,

select l, count(*) over (order by l nulls first)
from (values ([NULL::int]), ([1]), ([NULL::int])) t(l)

returns 3, 3, 3. With this PR it returns the correct 2, 2, 3, because partial_cmp_list orders a NULL element the opposite way from the sort. List<Int> keys are much more common than lists of lists, so this is probably the most practical benefit of the change.

[viirya]

Could this be simpler? GROUPS frames already find peers with ScalarValue equality (check_equality / search_in_slice), and ScalarValue::eq goes through nested_eq for lists, structs and maps. Reusing that would avoid the cache, the prune_state hook, the manual auto-trait impls and the custom Debug, at the same per-row cost as the existing non-nested path. If there is a semantic reason it doesn't work here (dictionary nulls?), a comment saying so would help. If the cache stays, I think the general reason is that building a comparator per row is costly, and O(n) for dictionaries. The comment currently gives only the dictionary case.

[sunchao]

I kept Arrow comparison because scalar equality can give wrong results for lists containing dictionary values. Two rows can both represent [NULL], with one using a null dictionary key and the other a key pointing to a null value. ScalarValue equality distinguishes them; Arrow treats them as peers. Added a regression and explained this beside the scan. The cache comment now covers comparator setup generally, including deriving logical nulls across dictionary arrays.

Updated in 9910027.

[viirya]

Which public type does the downstream catch_unwind reproducer need to stay UnwindSafe? WindowAggState holds an ArrayRef (out_col), and WindowFrameContext::Groups holds ScalarValues, so by the auto-trait rules neither looks UnwindSafe already. That would leave WindowFrameStateRange on its own (constructible through Default) as the only type affected. If that's the case, these manual impls may not be buying much.

[sunchao]

Yes, the public type affected is WindowFrameStateRange. Compile controls confirm that it supports both borrowed and owned catch_unwind usage before this PR and with the two marker implementations, and loses that compatibility without them. I kept the two implementations on the private immutable comparator wrapper and updated the comment to name the public type precisely. The guarantee does not extend to WindowAggState or the Groups variant.

Updated in 9910027.

[viirya]

"Arrow's sort comparator also handles dictionary nulls" reads as if dictionary keys take this path, but only nested types (including dictionary-of-nested) do. Could this say what problem it refers to?

[sunchao]

Clarified the exact case: dictionary encoding inside a nested list, where a null key and a key referencing a null value represent the same logical list element. The accompanying regression covers this distinction.

Updated in 9910027.

[viirya]

The .max(idx) is load-bearing here, unlike in the ordered search below. An equality scan stops at the first non-peer, so starting before idx would return immediately. Worth a short comment.

[sunchao]

Added a comment explaining that an equality scan cannot skip preceding non-peers when evaluation starts partway through a partition.

Updated in 9910027.

[viirya]

nit: if is_peer == SIDE { break; } is easier to read than the nested if if.

[sunchao]

Changed this to if is_peer == SIDE { break; }.

Updated in 9910027.

[viirya]

This test asserts implementation details: cache identity through Weak::upgrade, and the exact point where memory is released. That will break under harmless refactors. The result correctness after pruning (the last assertion) seems like the part worth keeping.

[sunchao]

Removed the redundant assertions about the old comparator being released. Kept one check that the comparator is reused and one that pruning releases the old array, alongside the result check: repeated setup and retained arrays would otherwise escape the value assertions. Renamed the test to describe those behaviors.

Updated in 9910027.

[viirya]

The removed text explained why these types are rejected: the peer check went through compare_rows. For nested keys that is no longer true, so for example ORDER BY d, l would now be computed correctly but is still rejected. I agree with keeping admission unchanged in this PR, but the comment should say these restrictions are now conservative and could be relaxed, ideally linking a follow-up issue, rather than just restating the rule. (Same for is_list_type below.)

[sunchao]

Updated these comments to say that SQL admission remains conservative and can be relaxed separately after validating the additional types. The documentation now distinguishes the admission restrictions from Arrow's nested peer comparison support.

Updated in 9910027.

[viirya]

Could we add the plain-list case that this PR fixes (see the review summary), e.g. order by l nulls first over [NULL], [1], [NULL]? It returns 3, 3, 3 on main and 2, 2, 3 here.

[sunchao]

Added this plain-list case to window.slt with counts 2,2,3 and included the wrong-result fix in the PR rationale. Restoring the old peer-search behavior in a control reproduces 3,3,3.

Updated in 9910027.

[viirya]

This adds a new integration test binary, which means another link step in CI. Could these tests live under core_integration (e.g. a module in tests/physical_optimizer or tests/execution) instead?

[sunchao]

Moved the tests into tests/execution/window_nested_range.rs and registered the module in the existing core_integration target. The separate integration-test binary is gone.

Updated in 9910027.

[viirya]

Only InputOrderMode::Sorted is covered. Adding a Linear case with a PARTITION BY column would catch the is_row_ahead failure mentioned in the summary.

[sunchao]

Added Linear coverage for frames ending at CURRENT ROW for all three nested key types in the existing tests. A newer row in a second partition must now release completed peers in the first before EOF, with the ordering requirement checked. The fix uses Arrow comparison and reuses its Boolean result within each partition evaluation, avoiding repeated comparator setup without persistent state. The genuine PartiallySorted SQL control passes already; that stream path does not populate most_recent_row, so it does not reach this scalar comparison with a row bound.

Updated in 9910027.

[comphead]

Thanks for the update. The Arrow comparator is a good fit for peer detection. Points not raised earlier:

• is_row_ahead is shared with GROUPS frames, but GROUPS group boundaries still come from ScalarValue equality (check_equality in WindowFrameStateGroups). The nested dictionary null case described here would still split into separate groups there, unlike RANGE peers. A follow-up issue or a sentence in the description would make the scope clear. I did not run this case.
• Design question: peer groups are contiguous equal runs, which evaluate_partition_ranges (arrow partition) already computes in one vectorized pass for PARTITION BY and rank. Caching those ranges per array instead of a comparator would turn the scan into a lookup and might remove the custom Debug and the unwind-safety impls. I have not checked that partition handles the dictionary-inside-list null case, so the existing regression would need to confirm it.
• Nit: peer_comparator ends with .as_ref().unwrap() right after assigning the value. Option::insert on the rebuild branch avoids an unwrap in library code.
• Nit: column.data_type().is_nested() runs for every row on both the start and end search, and it recurses through dictionary and run-end wrappers. Deciding once per calculate_range, or when the cache is built, avoids the repeated work.

[comphead]

Nothing in production shares this comparator, so the Arc looks like it exists only for the unit test, which observes reuse through Arc::downgrade. Owning the LexicographicalComparator directly and checking reuse through behavior would keep the type shaped by the runtime need.

[comphead]

The plan setup (create_window_expr, the exec construction and the ordering_satisfy_requirement assertion) is repeated almost verbatim in nested_range_emits_completed_peers_before_eof. A small helper taking the mode and frame would remove about 40 lines.

Also, the WindowAggExec and Sorted cases for the List<List> and List<Struct> keys are reachable from SQL, since a list is accepted as the first ORDER BY key (ORDER BY l, tie). They would fit window.slt with set datafusion.execution.batch_size = 1 to get the batch-boundary crossing. The struct key, Linear mode and the pending-stream case need to stay in Rust.

[comphead]

Every case orders ascending. is_row_ahead now depends on the direction through is_lt() with the sort options, so one DESC case with the opposite null placement in this streaming test would pin that down.

[comphead]

Both nested queries here use distinct keys, so they only show that the Uncomparable values error is gone. They do not exercise peers, which the comment above them claims. Adding a tied row, for example ([[1]]), ([[1]]), ([[2]]) expecting 2, 2, 3, and the same for [struct(1)], would cover it.