Skip to content

Fix the return dtype of grouped is_sorted and is_constant - #11238

Merged
connortsui20 merged 2 commits into
developfrom
ct/grouped-is-sorted-finalize
Nov 4, 2026
Merged

connortsui20 merged 2 commits into
developfrom
ct/grouped-is-sorted-finalize

Conversation

@connortsui20

Copy link
Copy Markdown
Member

Summary

Every grouped is_sorted and is_constant call fails with "Return DType mismatch". Both finalize methods read the result field out of the nullable partial struct, which gives Bool? while the return dtype is a non-nullable Bool. #11188 found the is_sorted case, and its grouped pushdown test covers only min_max because of it.

Changes

Both finalize methods now fill null partials. A null partial comes from an empty or null group and is the empty state, so each fill uses the verdict that finalize_scalar already gives that state: true for is_sorted (vacuously sorted) and false for is_constant. Grouped and scalar results now agree. Note that a null group gets the same verdict as an empty group, since the partial can't tell them apart and the return dtype is non-nullable.

Each aggregate gets a grouped test that puts empty, all-null, and null groups next to ordinary ones.

`GroupedAccumulator<IsSorted>::finish` failed with "Return DType
mismatch". `IsSorted::finalize` read `is_sorted` out of the nullable
partial struct, so it returned `Bool?` instead of the non-nullable
`Bool` return dtype. Empty and null groups leave a null partial, which
is the empty state. Grouped `is_sorted` now returns `true` for these
groups, the same verdict that `finalize_scalar` gives the empty state.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
`GroupedAccumulator<IsConstant>::finish` failed with "Return DType
mismatch". `IsConstant::finalize` read `is_constant` out of the
nullable partial struct, so it returned `Bool?` instead of the
non-nullable `Bool` return dtype. Empty and null groups leave a null
partial, which is the empty state. Grouped `is_constant` now returns
`false` for these groups, the same verdict that `finalize_scalar`
gives the empty state.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 added the changelog/fix A bug fix label Nov 4, 2026
@connortsui20
connortsui20 marked this pull request as ready for review November 4, 2026 18:03
@codspeed

codspeed Bot commented Nov 4, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 22.47%

1 warning

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚡ 1 improved benchmark
✅ 1769 untouched benchmarks
⏩ 105 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ CPU Simulation missing_stats[16] 92.8 µs 75.8 µs +22.47%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/grouped-is-sorted-finalize (47a34bd) with develop (3be54de)

Open in CodSpeed

Footnotes

  1. 105 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@connortsui20
connortsui20 merged commit 4addfba into develop Nov 4, 2026
150 checks passed
@connortsui20
connortsui20 deleted the ct/grouped-is-sorted-finalize branch November 4, 2026 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants