Skip to content

Move array serde from VTable to standalone ArrayPlugin impls - #10111

Closed
mhk197 wants to merge 2 commits into
developfrom
mk/standalone-array-plugins
Closed

mhk197 wants to merge 2 commits into
developfrom
mk/standalone-array-plugins

Conversation

@mhk197

@mhk197 mhk197 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Remove the blanket impl<V: VTable> ArrayPlugin for V.
  • Remove serialize / deserialize from the VTable trait. The vtable now only covers in-memory behaviour.
  • Every serializable encoding implements ArrayPlugin directly on its vtable type (impl ArrayPlugin for Primitive, impl ArrayPlugin for FoR, ...). There are no new plugin structs, and session.arrays().register(X) is unchanged.
  • Each impl lives in a plugin.rs child module of the file that holds impl VTable for X, e.g. primitive/vtable/plugin.rs or alp/array/plugin.rs. Because it is a child module, it can use the parent's private metadata types without visibility changes.
  • The old serde bodies are ported verbatim, so the wire format is unchanged.

Encodings that were never serializable lose the two methods and get no plugin: Filter, Slice, Shared, Interleave, ScalarFn, ForeignArray, TransposedBool, PiecewiseSequence and the test-harness vtables.

  • The standalone DecimalBytePartsPlugin struct is removed. Its v1/v2 serde now lives in impl ArrayPlugin for DecimalByteParts, and initialize registers DecimalByteParts. The bare_vtable_refuses_serde test is deleted because its premise no longer holds.
  • ALPPatchedPlugin and BitPackedPatchedPlugin are unchanged apart from delegating to ArrayPlugin::deserialize(&ALP/&BitPacked, ...).

Behaviour changes

  • PiecewiseSequence is no longer registered in ArraySession::default(), and TransposedBool is no longer registered by vortex_fastlanes::initialize. Neither could serialize or deserialize before. Serializing them now fails with "not registered for serialization" instead of "does not support serialization", and serialization_is_not_supported is updated to match.
  • PythonVTable gets a minimal ArrayPlugin whose serialize returns Ok(None) and whose deserialize bails, the same as before. This keeps vortex._lib.registry.register working.

@codspeed

codspeed Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Merging this PR will degrade performance by 13.28%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 4 regressed benchmarks
✅ 2057 untouched benchmarks
⏩ 503 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation density_sweep_single_slice[0.9] 32.1 µs 48.3 µs -33.57%
❌ Simulation sum_v2_i64 193.9 µs 224.7 µs -13.7%
❌ Simulation sum_i64 194 µs 224.4 µs -13.56%
❌ WallTime dict_canonicalize_gt_u8_neon[1000000] 488.7 µs 547.2 µs -10.68%
⚡ WallTime compare_u64_avx2 4.1 µs 3.7 µs +10.8%

Tip

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


Comparing mk/standalone-array-plugins (e50242a) with develop (292cf6f)

Open in CodSpeed

Footnotes

  1. 503 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. ↩

@mhk197 mhk197 closed this Oct 12, 2026
mhk197 added a commit that referenced this pull request Nov 6, 2026
## Summary

Removes the blanket `impl<V: VTable> ArrayPlugin for V` and the
`serialize` / `deserialize` methods on `VTable`. Every serializable
encoding now implements `ArrayPlugin` explicitly on its vtable type, in
a `plugin.rs` child module next to its `impl VTable`. Redo of #10111 on
current develop.

## Changes

- `VTable` covers only in-memory behaviour; serde is `impl ArrayPlugin
for X`.
- Serde bodies are ported verbatim, so wire formats are unchanged.
- `read_ids` / `write_ids` default to `vec![self.id()]`, what the
blanket impl declared. Plugins with one wire ID no longer spell them
out, including `ALPPatchedPlugin`, `ScalarFnArrayPlugin` and the Python
stub.
- Inside an impl the wire ID is `ArrayPlugin::id(self)`; only the `id`
method itself delegates to `VTable::id`.
- `session.arrays().register(X)` is unchanged for every encoding that
was serializable.
- In-memory-only vtables lose the two stub methods and get no plugin:
Filter, Slice, Interleave, ScalarFn, ScalarFnValidity, ForeignArray,
PiecewiseSequence, Widen, Shared, test-harness vtables.
- `DecimalPlugin` (#10995) becomes `impl ArrayPlugin for Decimal`: one
wire ID, same legacy-node reads, no separate type. Its module,
`StoredDecimal` and tests stay where they are.
- Multi-ID plugins (`BitPackedPlugin`, `FoRPlugin`,
`DecimalBytePartsPlugin`) and `WidenPlugin` are untouched;
`ALPPatchedPlugin` now delegates to `ArrayPlugin::deserialize(&ALP,
..)`.
- `PythonVTable` keeps a stub plugin so the Python `register` API still
compiles.
- Vtable files that gained a `plugin.rs` child are directory modules
(`array.rs` becomes `array/mod.rs`, 14 pure renames). ALP's own impl and
`ALPPatchedPlugin` share `alp/array/plugin.rs`.
- Tests that called the vtable serde directly (ALP, ALPRD, Dict, List,
RunEnd, VarBin, RLE, serde.rs, unary, zstd buffers) go through
`ArrayPlugin` instead.

| | count |
|---|---|
| new `impl ArrayPlugin` (vortex-array) | 20 |
| new `impl ArrayPlugin` (encodings + python) | 17 |
| vtables that only lose the stub methods | 14 |

## API changes

- `VTable` loses `serialize` and `deserialize`. An external vtable that
was registered through the blanket impl must now `impl ArrayPlugin` to
be registrable.
- `ArrayPlugin::read_ids` / `write_ids` gain defaults of
`vec![self.id()]`.
- `vortex_array::arrays::DecimalPlugin` is removed; register `Decimal`
instead.

## Behaviour changes

- `PiecewiseSequence` is no longer registered by
`ArraySession::default()`; it never serialized, and its error is now "is
not registered for serialization".
- The writer seeds the footer's encoding list from the session registry,
so dropping `vortex.piecewise-sequence` makes every default-session file
48 bytes smaller. `test_stock_ticker_file_size` expectations move
accordingly (70060 → 70012, 69996 → 69948, 55144 → 55096).
- `DecimalByteParts`, `BitPacked` and `FoR` can no longer be registered
bare, so `bare_vtable_refuses_serde` and `vtable_serde_requires_plugin`
are removed.

## Testing

- `cargo clippy --all-targets --all-features -- -D warnings` and `cargo
+nightly-2026-09-10 fmt --check` on every touched crate (vortex-array,
vortex-file, vortex-layout, vortex-python and the 13 encodings crates).
- `cargo nextest run` on all touched crates plus vortex-ipc and
vortex-scan: 8981 passed, 0 failed; `cargo test --doc` on vortex-array,
fastlanes, decimal-byte-parts and alp.
- `cargo nextest run -p vortex-file -E
'test(test_stock_ticker_file_size)'` with and without `--features zstd`,
to derive the new sizes.

## Follow-up

- Pre-existing, unchanged here: the Patched, RLE, OnPair, ALPRD, FSST
and Zstd deserializers index children or buffers before checking their
counts, so a malformed node panics in `SerializedArray::child` instead
of returning an error. Worth hoisting the count checks in a separate PR.
- Fold `FoRPlugin` / `BitPackedPlugin` / `DecimalBytePartsPlugin` into
`impl ArrayPlugin for FoR` etc. if wanted.
- Overlaps with #11298, which makes the same change with inline impls
and trait defaults for `read_ids` / `write_ids`.

---------

Signed-off-by: Matt Katz <[email protected]>
@robert3005
robert3005 deleted the mk/standalone-array-plugins branch November 10, 2026 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant