Conversation
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Merging this PR will degrade performance by 10.18%
Performance Changes
Comparing Footnotes
|
Contributor
|
why do we want to do this? |
Contributor
Author
|
I've put a bit of a description in the PR now. |
a10y
approved these changes
Mar 26, 2026
gatesn
added a commit
that referenced
this pull request
Mar 26, 2026
See #7181 --------- Signed-off-by: Nicholas Gates <nick@nickgates.com>
gatesn
added a commit
that referenced
this pull request
Mar 26, 2026
See #7181 Signed-off-by: Nicholas Gates <nick@nickgates.com>
gatesn
added a commit
that referenced
this pull request
Mar 26, 2026
See #7181 --------- Signed-off-by: Nicholas Gates <nick@nickgates.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This introduces the end-state struct
Array<V>per the vtables design doc: https://docs.vortex.dev/developer-guide/internals/vtablesIt keeps the old ArrayAdapter around for now, to be removed at a later date.
Array VTable Migration Plan
Context
The Array vtable system is the last major plugin point that hasn't been migrated to the unified
vtable pattern described in
docs/developer-guide/internals/vtables.md. ScalarFn, AggregateFn,and ExtDType already follow the target pattern. Array is the largest migration (~40 encodings
across vortex-array, encodings/*, vortex-python, vortex-cuda).
Current state:
VTabletrait +ArrayAdapter<V>(#[repr(transparent)]) +DynArraypublicsealed trait +
vtable!macro. Each concrete array struct (e.g.PrimitiveArray) owns dtype, len,stats and derefs to
dyn DynArrayvia unsafe casts throughArrayAdapter.Target state:
ArrayVTabletrait +Array<V>generic data struct (owns dtype, len, stats,derefs to
V::Array) + sealedDynArraythin forwarder +ArrayRefnewtype. Follows theFooRef → DynFoo → Foo<V> → FooVTabledispatch chain.Phase 1: Introduce
Array<V>, rename trait, change signaturesGoal: Replace the core VTable machinery in one atomic change. All encodings switch from
ArrayAdapter<V>toArray<V>at once since VTable method signatures change from&Self::Arrayto
&Array<Self>.This is a large but mechanical change — each encoding's update follows a predictable recipe.
1.1 Rename
VTable→ArrayVTablevortex-array/src/vtable/mod.rs: rename traitimpl VTable for→impl ArrayVTable for,V: VTable→V: ArrayVTable, etc.1.2 Define
Array<V: ArrayVTable>New file
vortex-array/src/vtable/typed.rs:Inherent methods on
Array<V>:dtype(),len(),is_empty(),statistics(),encoding_id(),into_inner(), constructors.1.3 Change
ArrayVTablemethod signaturesAll VTable methods change from
&Self::Arrayto&Array<Self>:Since
Array<V>derefs toV::Array, encoding impls that accessarray.bufferetc. continueto work unchanged via deref. The method signature annotation is the only change in most cases.
Remove
fn vtable(array: &Self::Array) -> &Self— vtable now stored inArray<V>.1.4 Implement
DynArrayforArray<V>Replace
impl<V: VTable> DynArray for ArrayAdapter<V>withimpl<V: ArrayVTable> DynArray for Array<V>. This is the blanket forwarder — all the logic that currently lives in theArrayAdapterimpl (bounds checking, stat propagation, slice/filter/take wrapping) moves toArray<V>inherent methods, and the DynArray impl becomes thin forwarders.Also move these trait impls from
ArrayAdapter<V>toArray<V>:ArrayHash,ArrayEqArrayVisitorReduceNodeprivate::Sealed1.5 Migrate all encodings
Per-encoding recipe (example:
Primitive):Hoist common fields out of
PrimitiveArray:dtype: DType→ now inArray<V>stats_set: ArrayStats→ now inArray<V>PrimitiveArraykeeps only:buffer: BufferHandle,validity: ValidityUpdate VTable impl signatures:
Move constructors to vtable ZST:
PrimitiveArray::new(buffer, ptype, validity)→Primitive::new(buffer, ptype, validity) -> ArrayRefArray<Primitive>for typed constructionPrimitiveArrayas the inner type with encoding-specific methodsRemove
vtable!(Primitive)— no longer needed.Array<V>handlesIntoArrayvia itsown
impl IntoArray for Array<V>.Remove
PrimitiveArray'sDereftodyn DynArray— this was generated byvtable!andis no longer needed.
Array<V>implements DynArray directly.1.6 Update
Matcher1.7 Remove old machinery
ArrayAdapter<V>structvtable!macroMatcher for Vimpl that usedArrayAdapterdowncast/downcast_owned/upcast_arrayhelpers — no more#[repr(transparent)]tricks needed; use standard
Arc::downcast::<Array<V>>()Key files modified
vortex-array/src/vtable/mod.rsvortex-array/src/vtable/typed.rsvortex-array/src/vtable/dyn_.rsvortex-array/src/vtable/operations.rs&V::Array→&Array<V>vortex-array/src/vtable/validity.rs&V::Array→&Array<V>vortex-array/src/array/mod.rsvortex-array/src/arrays/*/vtable/*.rsvortex-array/src/arrays/*/array/*.rsencodings/*/src/*.rsvortex-python/src/arrays/py/vtable.rsvortex-cuda/src/layout.rsVerification
cargo buildacross entire workspacecargo testacross entire workspacecargo clippy --all-targets --all-featurescargo +nightly fmt --allcargo xtask public-api(public API changes expected — newArray<V>, renamed trait)Phase 2: Clean up erased layer
Goal: Migrate
ArrayReffrom type alias to newtype struct. Move public API fromimpl dyn DynArraytoimpl ArrayRef. MakeDynArrayprivate. IntroduceArrayPlugin.This can be done incrementally after Phase 1.
2.1 Make
ArrayRefa newtypeAdd
Deref<Target = dyn DynArray>onArrayRefso existing call sites that usearray_ref.len()etc. continue to work. This provides a compat bridge while we migrate callers.
2.2 Move
impl dyn DynArraymethods toimpl ArrayRefMethods currently on
impl dyn DynArray + '_:is::<V>(),as_::<V>(),as_opt::<V>(),try_into::<V>()→impl ArrayRefas_constant(),nbytes(),is_arrow(),is_canonical()→impl ArrayRefwith_child()→impl ArrayRefRemove
impl DynArray for Arc<dyn DynArray>forwarding impl (no longer needed with newtype).2.3 Make
DynArrayprivateChange
DynArraytopub(crate). External callers useArrayRefmethods only.The sealed trait becomes truly internal plumbing.
2.4 Introduce
ArrayPluginReplace
DynVTablerole in registry withArrayPlugintrait following the pattern fromScalarFn's plugin:
Update
ArraySessionregistry to useArrayPlugininstead ofDynVTableRef.2.5 Update
Matcherreturn type (optional, separate step)Change
Match<'a>from&'a V::Arrayto&'a Array<V>. This gives callers access to commonfields (dtype, len, stats) directly on the typed handle, plus deref to
V::Arrayforencoding-specific fields.
This is a breaking change for any code that explicitly annotates the match type, but most code
uses type inference and gets
V::Arraymethods via deref anyway.Key files modified
vortex-array/src/array/mod.rsvortex-array/src/vtable/dyn_.rsvortex-array/src/session/mod.rsvortex-array/src/matcher.rsArc<dyn DynArray>→ArrayRef(mostly handled by newtype + Deref)Verification
Same as Phase 1, plus:
vortex-python)cargo test -p vortex-file)Risk Areas
Size of Phase 1 — touching ~40 encodings in one PR is large. Consider splitting into
sub-PRs: (a) infrastructure (Array, trait rename, DynArray blanket), (b) in-tree encodings,
(c) external encodings. The repo won't compile between (a) and (b).
Unsafe code removal —
ArrayAdapterused#[repr(transparent)]for zero-cost transmutes.Array<V>has multiple fields soArc::downcastreplaces transmute. Verify no performanceregression in downcast-heavy paths (e.g., execution engine).
Constructor migration — moving constructors from
PrimitiveArray::new()toPrimitive::new()touches many call sites across the workspace.DynArray for Arc<dyn DynArray>— removing this forwarding impl in Phase 2 may breakgeneric code that passes
ArrayRefwhere&dyn DynArrayis expected. Audit usage patterns.External encoding crates —
encodings/*are in-repo but separate crates. They'll needcoordinated updates in Phase 1.
Python/CUDA bindings — these have special vtable implementations (non-ZST vtables for
language bindings). Verify
Array<V>works correctly with non-ZST V.