Skip to content

Return extension values from ExtensionStoragePushdown - #11188

Closed
connortsui20 wants to merge 2 commits into
developfrom
ct/storage-pushdown-results
Closed

connortsui20 wants to merge 2 commits into
developfrom
ct/storage-pushdown-results

Conversation

@connortsui20

Copy link
Copy Markdown
Member

Summary

ExtensionStoragePushdown returned the results of its aggregate over storage as they were, so vortex.min_max[vortex.timestamp] gave raw i64 values without the time unit or time zone. Plain min_max returns timestamps for the same column, and nothing restores the type later, so the pushdown now returns extension values too.

Changes

Through a new StorageResultMapping trait, MinMax and IsSorted name their value fields, the fields of their partials and results that hold input values. They name them by index, since a dtype can't tell them apart: over Bool storage, the is_sorted field also has the storage dtype. Grouped finalize receives partials whose value fields already hold extension values, so the trait also requires that finalize only moves or drops those fields and ignores args.dtype, as both aggregates already do.

The fixed mapping is a stopgap until #11115 gives every aggregate that runs on storage a result mapping through ExtensionUse::Storage. The impls sit next to the aggregates, so #10769 can move them along.

Tests compare the pushdown with plain min_max over flat, chunked, constant, dictionary-encoded, and grouped input, and check that an is_sorted partial with a null value survives its round trip through a scalar.

API Changes

ExtensionStoragePushdown<A, E> now requires A: StorageResultMapping, which only MinMax and IsSorted implement. The value fields of the results and partials, such as min and max of vortex.min_max[vortex.timestamp], now have the extension dtype, and a storage value that the extension type rejects is an error, as with plain min_max.

`ExtensionStoragePushdown` lent its own return and partial dtypes to the
aggregate that it runs on storage. The next commit gives the fields of
those dtypes that hold input values the extension dtype again, so the
storage aggregate needs dtypes of its own, resolved over the storage
dtype.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
`ExtensionStoragePushdown` returned the results of its aggregate over
storage values unchanged. So `vortex.min_max[vortex.timestamp]` gave raw
`i64` values without the time unit or zone, while plain `min_max` gives
timestamps.

The new `StorageResultMapping` trait names the value fields of a partial
and a result, which hold input values, and `MinMax` and `IsSorted`
implement it. The pushdown requires the trait, so an aggregate without a
mapping does not compile. Partial states stay in storage form in memory.
`to_scalar` and `finalize_scalar` give their output the extension dtype,
`partial_from_scalar` removes it, and `finalize` receives states that
already have it.

The tests now compare the pushdown with plain `min_max` over flat,
chunked, constant, dictionary-encoded, and grouped input, and check that
an `is_sorted` partial with a null first value survives its round trip
through a scalar. The test that registered the pushdown by hand is gone,
because the default session test already checks the registration.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 added the changelog/break A breaking API change label Nov 3, 2026
@connortsui20
connortsui20 marked this pull request as ready for review November 3, 2026 22:55
@connortsui20
connortsui20 marked this pull request as draft November 3, 2026 22:56
@codspeed

codspeed Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 0.81%

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
❌ 1 regressed benchmark
✅ 1763 untouched benchmarks
⏩ 611 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ CPU Simulation random_i8[0.01] 99.6 µs 113.3 µs -12.09%
⚡ CPU Simulation take[(16, 8, 1)] 120.8 µs 107.9 µs +11.92%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/storage-pushdown-results (8d58672) with develop (bc58587)

Open in CodSpeed

Footnotes

  1. 611 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 mentioned this pull request Nov 3, 2026
10 of 47 tasks
connortsui20 added a commit that referenced this pull request Nov 4, 2026
TS (#10953) merged on 2026-11-03, and SN1 (#11183) and V10 (#11187)
merged on 2026-11-04. The dependency graph marks the three merged and
moves its state date to 2026-11-04. It gives SD, DP, FD and U9 their
open PRs (#11184, #11185, #11186 and #11188), and it drops #10408,
which closed without a merge. The text of parts 4 and 5 and the Fix
lines of EXT-V9, EXT-V10, EXT-N2 and EXT-S3 now record the same state.

Also record that AX, DS and SN2 are implemented on branches without a
PR, and add the approved one-pass design for the masked nulls of SN2.

Part 2 and decision X31 also record the fixes: #10953 made every i64
legal, #11183 made the spatial validators reject nullable nested
levels, #11187 gave the Date display its fallback, and #10408 closed.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20 added a commit that referenced this pull request Nov 4, 2026
## 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.

---------

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20

Copy link
Copy Markdown
Member Author

Closing in favor of #11243. Naming the value fields by index is a half-measure, and the claims work in #11115 replaces the adapter anyway, so the bug gets fixed there. The branch stays up, since its tests compare the pushdown with plain min_max and are useful for SP4.

connortsui20 added a commit that referenced this pull request Nov 4, 2026
The maintainer closed #11188 on 2026-11-04, because naming the value fields
by index is a half-measure and SP4 removes the adapter anyway. Issue #11243
tracks the bug until SP4. X12, part 4, the plan, and the findings now say
so, and part 3 asks whether SP1 needs a positional `Fields` mapping at all.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 deleted the ct/storage-pushdown-results branch November 4, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant