refactor(array): deprecate the one-off scalar and validity accessors - #9876
Draft
joseph-isaacs wants to merge 9 commits into
Draft
joseph-isaacs wants to merge 9 commits into
joseph-isaacs wants to merge 9 commits into
Conversation
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Replace inline type-erased storage with a lazy boxed implementation and leave a FIXME to revisit inline storage only if benchmarks justify it. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
`OperationsVTable` is already imported in scope, so the explicit `crate::vtable::...` target duplicates the label and trips `rustdoc::redundant_explicit_links` under `-D warnings`. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEKHRLFEn9Tc7mHcRG7Hc9
`ArrayRef::probe` supersedes the one-off scalar and validity accessors: a probe performs the same bounds and validity checks but can retain encoding preparation across lookups, which the one-shot methods cannot. Deprecate `execute_scalar`, `is_valid`, and `is_invalid` on both `ArrayRef` and `Array<V>`, and repoint the already-deprecated `scalar_at` notes at `probe` rather than at `execute_scalar`. Add `Array::<V>::probe` so the typed handle has the replacement the notes name. On the encoding side, deprecate `OperationsVTable::scalar_at` in favour of `probe_scalar`, which receives a `ProbeAccess` for retained state. The default `probe_scalar` still forwards to it during migration. Signed-off-by: "Joe Isaacs" <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMRyEJHyh72KkM8o9paBeP
… site Deprecating the one-off scalar and validity accessors leaves several hundred call sites warning across the workspace, and CI runs clippy with `-D warnings`. Annotate each site with `#[expect(deprecated)]` rather than allowing the lint globally, so every suppression is visible in the code that needs it and goes unfulfilled once that caller moves to `ArrayRef::probe` — the lint then points at the annotation to delete. Sites were located by the compiler rather than by grep: each crate was checked with `--all-targets --all-features`, the `deprecated` diagnostics parsed from the JSON output, and an annotation placed on the enclosing item. Three cases need something other than the enclosing function: - `#[rstest]` discards lint attributes from its expansion, so attributes between `#[case]` and `fn`, above `#[rstest]`, inside the body, and on individual statements all leave the warning firing. Those files take a `#![expect(deprecated)]` at file or `mod tests` scope instead. - A `macro_rules!` body is linted at its definition site, so the annotation belongs in the macro. One in `assert_nth_scalar!` clears the warnings it caused in five downstream encoding crates. - A deprecated item using another deprecated item still warns, so `scalar_at` and `is_invalid` carry their own annotation. Also reflows the two `#[deprecated]` notes from the previous commit that rustfmt wanted on one line. Signed-off-by: "Joe Isaacs" <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMRyEJHyh72KkM8o9paBeP
`ArrayRef::probe` retains preparation across scalar lookups, but `Validity` had no equivalent: its per-row accessors execute the validity array from scratch on every call, and the only `ValidityProbe` a caller could reach came from `ProbeAccess::validity` inside an encoding's `probe_scalar`. Add `Validity::probe`, taking the same `ProbeUsage` as the array probe and returning an owned `ValidityProbe`. Constant states answer from the variant alone; `Validity::Array` reads through an owned `ArrayProbe`, so `Repeated` keeps its preparation instead of rebuilding it per row. `ValidityProbe` gains an `Owned` access variant alongside `Once` and `Repeated`, and `execute_is_valid`/`execute_is_invalid` next to the existing `execute_scalar`. Its `len` becomes optional: a probe borrowed from a parent still bounds-checks against the source array, while one built from a bare `Validity` has no length of its own, so constant states answer any index and array-backed validity is bounds-checked by its own probe. Signed-off-by: "Joe Isaacs" <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMRyEJHyh72KkM8o9paBeP
`Validity::probe` now offers the same per-row answers while retaining preparation across lookups, so the one-off accessors have a replacement they previously lacked. Deprecate `Validity::execute_is_valid` and `execute_is_null` in favour of `probe`, and repoint the already-deprecated `is_valid`/`is_null` notes at it too, since they named the accessors this commit deprecates. The replacement for a null check is `execute_is_invalid`, matching `ArrayRef::is_invalid` rather than the old `is_null` spelling. Signed-off-by: "Joe Isaacs" <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMRyEJHyh72KkM8o9paBeP
Annotate the callers of `Validity::execute_is_valid` and `execute_is_null` with `#[expect(deprecated)]`, so each suppression sits in the code that needs it and goes unfulfilled once that caller moves to `Validity::probe`. Sites were located by the compiler, as for the array accessors. One `#[rstest]` function takes a `#![expect(deprecated)]` at `mod tests` scope instead, since rstest discards lint attributes from its expansion. Signed-off-by: "Joe Isaacs" <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMRyEJHyh72KkM8o9paBeP
joseph-isaacs
force-pushed
the
ji/array-probe-api
branch
from
September 15, 2026 20:19
d025a09 to
0fbdafe
Compare
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.
Summary
Stacked on #9843, which adds the probe API. That PR left the one-off accessors in place with
TODO(joe): deprecate thisandFIXME: Deprecate scalar_at once encodings have migrated; this one discharges those markers.A probe performs the same bounds and validity checks as the one-off accessors but can retain encoding preparation across lookups, which a one-shot call cannot. Deprecating the one-off spellings points callers at the replacement without removing anything yet.
Changes
Deprecations.
execute_scalar,is_validandis_invalidon bothArrayRefandArray<V>;OperationsVTable::scalar_atin favour ofprobe_scalar, whose default still forwards to it during migration. The already-deprecatedscalar_atnotes namedexecute_scalar, so they are repointed atprobe.Array::<V>::probe.Array<V>derefs to the encoding data rather than toArrayRef, so the typed handle had noprobefor the deprecation notes to name.Validity::probe.Validityhad no equivalent ofArrayRef::probe— the onlyValidityProbea caller could reach came fromProbeAccess::validityinside an encoding'sprobe_scalar— soValidity::execute_is_validandexecute_is_nullhad no replacement to point at.Validity::probetakes the sameProbeUsageand returns an ownedValidityProbe: constant states answer from the variant,Validity::Arrayreads through an ownedArrayProbe.ValidityProbegains anOwnedaccess variant andexecute_is_valid/execute_is_invalid. Itslenbecomes optional, so a probe borrowed from a parent still bounds-checks against the source array while one built from a bareValidityrelies on the inner probe — matching whatValidity::execute_is_validdoes today. With that in place, both validity accessors are deprecated too; the replacement for a null check isexecute_is_invalid, matchingArrayRef::is_invalidrather than the oldis_nullspelling.416
#[expect(deprecated)]annotations across 191 files. CI runs clippy with-D warnings, so the deprecations have to be suppressed somewhere. Annotating each site rather than allowing the lint globally keeps every suppression visible in the code that needs it, andexpectrather thanallowmeans the annotation goes unfulfilled once that caller moves toprobe— the lint then points at the line to delete.Sites were located by the compiler, not by grep: each crate checked with
--all-targets --all-features,deprecateddiagnostics parsed from the JSON output, and an annotation placed on the enclosing item. Three cases need something other than the enclosing function:#[rstest]discards lint attributes from its expansion — attributes between#[case]andfn, above#[rstest], inside the body, and on individual statements all leave the warning firing. Those 9 files take a#[expect(deprecated)]at file ormod testsscope.macro_rules!body is linted at its definition site, so the annotation belongs in the macro. One insideassert_nth_scalar!clears the warnings it was causing in five downstream encoding crates.scalar_atandis_invalidcarry their own annotation.Commits are split so the deprecations can be reviewed separately from the annotation churn, and so the annotation commits can be dropped wholesale once callers migrate.
API Changes
Deprecations only — nothing is removed and no behavior changes. Additive:
Array::<V>::probe,Validity::probe, andValidityProbe::{execute_is_valid, execute_is_invalid}.Two items were considered and deliberately left alone:
ArrayRef::all_valid/all_invalidare whole-array stats queries rather than per-row access, and Python'sArray.scalar_atis a different audience with noprobeexposed.Verification
cargo check --all-targets --all-featureson 28 crates:deprecated=0 unfulfilled=0 errors=0.cargo +nightly-2026-09-10 fmt --all --checkclean.cargo clippy --all-targets --all-features -- -D warningsclean onvortex-array,vortex-fastlanes,vortex-parquet-variant,vortex-spatial,vortex-fuzz,vortex-pco,vortex-zstd,vortex-fsst,vortex-alp.cargo test --doc -p vortex-arraycovers the newValidity::probeexample.Not verified:
vortex-duckdbneeds libduckdb, which was unavailable in this environment. Its 3 annotated sites (exporter/run_end.rs, two induckdb/vector.rs) were placed by reading the code rather than from a compiler diagnostic, and are the only ones in the PR not confirmed by a build. Clippy was run on the heaviest crates only; the rest hadcargo check.🤖 Generated with Claude Code
https://claude.ai/code/session_01EMRyEJHyh72KkM8o9paBeP
Generated by Claude Code