Skip to content

Write Widen arrays through an array plugin - #11002

Closed
connortsui20 wants to merge 2 commits into
developfrom
ct/widen-serialize-canonical
Closed

connortsui20 wants to merge 2 commits into
developfrom
ct/widen-serialize-canonical

Conversation

@connortsui20

@connortsui20 connortsui20 commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Folded into #10946. The replacement writes a concrete decimal leaf in the existing vortex.decimal wire form without executing arrays during serialization. Unsupported Widen inputs require explicit caller preparation. The original branch is retained for recovery.

Original proposal

Tracking Issue: #10701

Stacked on #11057. #10946 builds on it. No release gate.

Summary

Widen has no wire form yet (#10733), but IPC and writes without a compressor still need to store it. A writer plugin makes that work through the same session API used by direct metadata callers.

Changes

An integer Widen writes its canonical primitive form. Recursive canonicalization makes lazy child validity storable, while keeping the Widen as the serialization source preserves statistics in its logical dtype. Restricted contexts need only the existing primitive wire ID. Decimal writes follow in #10946.

API Changes

Adds public vortex_array::arrays::widen::WidenPlugin to the default ArraySession. Empty array sessions can register it explicitly. The plugin registers no reader because vortex.widen has no wire form.

@connortsui20
connortsui20 added this pull request to stack #11007 October 29, 2026 21:33
@connortsui20 connortsui20 changed the title ct/widen serialize canonical Write Widen arrays as their canonical arrays Oct 29, 2026
@codspeed

codspeed Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 15.61%

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

❌ 7 regressed benchmarks
✅ 2311 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ CPU Simulation filter_powerlaw_by_random[10000] 1.1 µs 1.5 µs -27.44%
❌ CPU Simulation bench_compare_varbin[(50000, 2)] 133.3 µs 161.5 µs -17.47%
❌ CPU Simulation bench_compare_varbin[(50000, 64)] 135.3 µs 163.1 µs -17.04%
❌ CPU Simulation bench_compare_sliced_dict_varbinview[(7500, 10000)] 199.8 µs 227.1 µs -12.03%
❌ CPU Simulation bench_compare_sliced_dict_varbinview[(10000, 10000)] 206.1 µs 233.3 µs -11.63%
❌ CPU Simulation bench_compare_sliced_dict_varbinview[(1000, 10000)] 78.1 µs 88.3 µs -11.62%
❌ CPU Simulation take_filter_primitive_small_uncached_random_mask_random_indices[4096, 10] 21.7 µs 24.3 µs -10.65%

Tip

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


Comparing ct/widen-serialize-canonical (7929610) with develop (ea94807)

Open in CodSpeed

@connortsui20
connortsui20 marked this pull request as ready for review October 29, 2026 21:44
@connortsui20 connortsui20 added the changelog/feature A new feature label Oct 29, 2026
Comment thread vortex-array/src/serde.rs Resolved
@connortsui20
connortsui20 force-pushed the ct/widen-serialize-canonical branch from de8a7a6 to f3d53cc Compare October 30, 2026 17:50
@connortsui20 connortsui20 changed the title Write Widen arrays as their canonical arrays Write Widen arrays through an array plugin Oct 30, 2026
@connortsui20
connortsui20 removed this pull request from stack #11007 October 30, 2026 18:02
@connortsui20
connortsui20 changed the base branch from develop to ct/array-plugin-read-write-ids October 30, 2026 18:03
@connortsui20
connortsui20 added this pull request to stack #11058 October 30, 2026 18:03
@connortsui20
connortsui20 force-pushed the ct/array-plugin-read-write-ids branch from 851cd14 to 9556b96 Compare October 30, 2026 19:28
@connortsui20
connortsui20 force-pushed the ct/widen-serialize-canonical branch from f3d53cc to 72749e5 Compare October 30, 2026 19:28
Comment on lines +42 to +60
fn serialize(
&self,
array: &ArrayRef,
session: &VortexSession,
) -> VortexResult<Option<ArraySerialization>> {
// TODO(connor)[Widen]: Add `vortex.widen` to both ID lists when it gets a wire form
// (#10733), and write it only where the writer permits it. This method cannot see the
// permitted IDs today.

// The child validity can be lazy, so the writer needs recursive canonicalization to
// produce nodes that it can store.
let canonical = array
.clone()
.execute::<RecursiveCanonical>(&mut session.create_execution_ctx())?
.0
.into_array();

session.array_serialize(&canonical)
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an open question: instead of doing this, do we want to have a more generalized version of this fix?

Not saying the PR should be blocked on this (as I do need to make progress on this stack), but there exist other encodings that we only have in-memory that we would want to ensure do not get serialized to disk (either because of an edition, or because we do not want them to be serialized ever).

Some examples:

  • Filter
  • Slice
  • Patched (the LLM is even telling me that this is a bug, and Patched needs a similar change as this one)
  • ScalarFn (which we serialize now but imo we might want a different name for serialized scalar functions, see Tracking Issue: Encodings vs Scalar Functions #9156)
  • Any encoding that might easily be created in memory but we disallow via editions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be in serialisation at this point we are waaaay too late. At this point we shouldn't execute but error and force the higher level to handle it. We need to do a normalisation pass which removes all non serialisable ids from the array tree

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh actually we could also just write the "old" decimal array here...

Not sure if that is better.

Base automatically changed from ct/array-plugin-read-write-ids to develop November 2, 2026 13:50
connortsui20 added a commit that referenced this pull request Nov 2, 2026
Tracking Issue: #10701

Bottom of the decimal `Widen` stack. #11002 builds on it. No release
gate.

## Summary

A plugin can need another encoding's wire form without taking over its
reader. `serialized_ids()` currently serves both jobs, so a `Widen`
writer cannot emit a narrow decimal node while preserving the decimal
reader.

## Changes

Reader registration now uses `read_ids()`, while serialization validates
its output against `write_ids()`. A writing session does not need a
reader for every ID it writes. Existing plugins keep their current IDs,
and tests cover reader ownership and the two validation contracts.
`LayoutPlugin` is unchanged.

## API Changes

Replaces `ArrayPlugin::serialized_ids()` with required `read_ids()` and
`write_ids()` methods, with no defaults. Read IDs stay ordered oldest to
newest.

---------

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
connortsui20 force-pushed the ct/widen-serialize-canonical branch from 72749e5 to 7929610 Compare November 2, 2026 13:50

@robert3005 robert3005 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not execute

Comment on lines +42 to +60
fn serialize(
&self,
array: &ArrayRef,
session: &VortexSession,
) -> VortexResult<Option<ArraySerialization>> {
// TODO(connor)[Widen]: Add `vortex.widen` to both ID lists when it gets a wire form
// (#10733), and write it only where the writer permits it. This method cannot see the
// permitted IDs today.

// The child validity can be lazy, so the writer needs recursive canonicalization to
// produce nodes that it can store.
let canonical = array
.clone()
.execute::<RecursiveCanonical>(&mut session.create_execution_ctx())?
.0
.into_array();

session.array_serialize(&canonical)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be in serialisation at this point we are waaaay too late. At this point we shouldn't execute but error and force the higher level to handle it. We need to do a normalisation pass which removes all non serialisable ids from the array tree

@connortsui20
connortsui20 removed this pull request from stack #11058 November 2, 2026 16:59
connortsui20 added a commit that referenced this pull request Nov 3, 2026
Tracking Issue: #10701

Based on `develop`, including the writer work previously in #11002.
#11003 and #10994 build on it. No release gate.

## Summary

Canonical decimal width follows precision, so small values under
`Decimal(38, 2)` still occupy 16 bytes each. A decimal `Widen` keeps
those values in a lower-precision child while retaining the column's
logical dtype.

## Changes

The child precision must hold every valid value and have a narrower
canonical width. Execution restores the logical width. Until the later
kernels land, operations use that canonical fallback.

Serialization writes a concrete decimal leaf through the existing
`vortex.decimal` wire form without executing values or validity. Integer
`Widen` arrays, non-decimal leaves, and nonserializable validity require
explicit preparation by the caller. Its metadata stores the width, while
the containing format supplies the parent precision. This keeps an `i32`
child at four bytes per value, including IPC and writes without a
compressor. A serialized-byte regression test checks that size
independently of the eventual read encoding.

<details>
<summary>Historical local benchmark results</summary>

These retained measurements predate this style pass and rebase. They do
not measure the current head.

`cargo bench -p vortex-array --bench widen_decimal`, Apple M4, medians.
The numbers were measured at the top of this stack before the split, so
they cover the whole stack, not this layer alone. The baseline is the
canonical input, and the candidate is the same values as a `Widen`
(child precision 5 under precision 10, 9 under 18, and 18 under 38).
CodSpeed on this PR measures the layer itself.

| bench | p10 canonical | p10 Widen | p18 canonical | p18 Widen | p38
canonical | p38 Widen |
| --- | ---: | ---: | ---: | ---: | ---: | ---: |
| `filter` | 22.16 µs | 5.083 µs | 13.66 µs | 4.916 µs | 10.58 µs |
24.74 µs |
| `execute` | 10.59 ns | 2.832 µs | 10.26 ns | 2.926 µs | 10.34 ns |
9.895 µs |

`execute` on a `Widen` is the widening copy, which the canonical input
does not need. The canonical `filter` medians at p10 and p18 were
bimodal across runs, and `filter` at p38 is slower on the `Widen`.

</details>

## API Changes

`WidenArray::try_new` accepts decimal children. Adds
`WidenArray::encode_decimal` and registers a writer-only `WidenPlugin`,
whose only write ID is `vortex.decimal`.

## Validation

Source review and rebase range-diffs only. No local tests, builds,
linting, or documentation checks were run. A commit hook flagged one
formatting wrap, corrected manually without rerunning the check.

---------

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
connortsui20 deleted the ct/widen-serialize-canonical branch November 6, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants