Skip to content

feat(array): scalar probes with one-off and repeated probe types - #9843

Merged
joseph-isaacs merged 4 commits into
developfrom
ji/array-probe-api
Sep 15, 2026
Merged

joseph-isaacs merged 4 commits into
developfrom
ji/array-probe-api

Conversation

@joseph-isaacs

@joseph-isaacs joseph-isaacs commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Adds reusable scalar probes with lazy state at each child slot. Repeated traversal of the same slot path reuses its preparation, including validity. ArrayRef::execute_scalar and is_valid now go through a one-off probe with the same bounds and validity checks.

Public API

// One-off read: borrows the array, retains nothing.
let value = array.probe().execute_scalar(10, &mut ctx)?;

// Repeated reads share preparation until the probe is dropped.
let mut probe = array.repeated_probe();
let first = probe.execute_scalar(10, &mut ctx)?;
let nearby = probe.execute_scalar(11, &mut ctx)?;
let valid = probe.execute_is_valid(12, &mut ctx)?;

ArrayProbe<'a> is the borrowed reader, either Once(&ArrayRef) or Repeated(&mut RepeatedArrayProbe). RepeatedArrayProbe owns its array handle and can outlive the original handle; it keeps the encoding's state, its validity, and one retained probe per child slot, all created on first use and dropped with the probe.

Encoding contract

pub trait OperationsVTable<V: VTable> {
    type ProbeState: Default + 'static;

    fn probe_scalar(
        state: &mut ProbeState<'_, V>,
        index: usize,
        ctx: &mut ExecutionCtx,
    ) -> VortexResult<Scalar> {
        Self::scalar_at(state.array(), index, ctx)
    }
...
}

Breaks

Need to add a type ProbeState: Default + 'static; to the OperationsVTable

@joseph-isaacs joseph-isaacs added the changelog/break A breaking API change label Sep 11, 2026
@codspeed

codspeed Bot commented Sep 11, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 15.59%

⚠️ 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

⚡ 4 improved benchmarks
❌ 7 regressed benchmarks
✅ 2187 untouched benchmarks
⏩ 218 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime words_gather_dispatch_avx512[1024] 9 ns 34 ns -73.53%
WallTime arrow_checked_add_u32_neon[16384] 13.5 µs 20.3 µs -33.42%
Simulation random_i8[0.5] 71.3 µs 94.9 µs -24.81%
Simulation new_raw_prim_test_between[i32, 2048] 64.1 µs 77.6 µs -17.43%
Simulation decompress[u64, (4000, 1024)] 71.5 µs 85.9 µs -16.71%
WallTime filtered_sink_i64_neon[NineNullsInTen] 15.4 µs 17.4 µs -11.42%
WallTime filtered_owned_i64_neon[NineNullsInTen] 15.3 µs 17 µs -10.17%
Simulation execute_scalar_struct_wide 503 µs 389.5 µs +29.15%
Simulation random_i16[0.8] 96.4 µs 78.5 µs +22.83%
Simulation execute_scalar_struct_simple 106.5 µs 89.6 µs +18.8%
Simulation allocate_drop_arrow[0] 456.9 ns 402.7 ns +13.45%

Tip

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


Comparing ji/array-probe-api (22e0f50) with develop (b5f43ba)

Open in CodSpeed

Footnotes

  1. 218 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.

@joseph-isaacs
joseph-isaacs added this pull request to stack #9860 September 14, 2026 12:02
@joseph-isaacs
joseph-isaacs marked this pull request as ready for review September 14, 2026 14:04
@robert3005

Copy link
Copy Markdown
Contributor

I will leave one comment instead of mentioning the lines. I think the name churn is unfortunate but I think it's fine. The If we have ProbeUsage::Once we should replace execute_scalar with probe usage once invocation. The null checks are split and duplicated right now and this replacement would force us to unify it now. I think it's right that you want to let implementation decide.

Not sure we need to hold arrayref in ProbeAccess::Once if we pass it to the function explicitly

///
/// The default preserves the existing scalar path without adding caching.
fn probe_scalar<'a>(
array: ArrayView<'a, V>,

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.

since you have this arrayview then ProbeAccess::Once state is weird but I could be missing something

@joseph-isaacs
joseph-isaacs marked this pull request as draft September 14, 2026 18:09
@joseph-isaacs

Copy link
Copy Markdown
Contributor Author

This was not ready to review yet

@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.

this addresses all the comments I had. The only question I had is whether we want to defer validity check to the impl instead of having default BUT I think if we fix validity computation with this it's better

joseph-isaacs and others added 3 commits September 15, 2026 18:51
…hell

Adds the vtable hook the scalar probe API builds on, without any of the
probe machinery. `OperationsVTable` gains `type ProbeState`, the state an
encoding may keep across repeated reads, and `probe_scalar(state, index,
ctx)`, defaulting to `scalar_at` so no encoding changes behaviour.
`ProbeState<'_, V>` for now only carries the `ArrayView`; the erased
dispatch routes one-off reads through it. Every encoding declares
`type ProbeState = ()`.

`scalar_at` stays until encodings migrate to `probe_scalar`.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CqrLKgPqFYGZK5sjk1qe7
Row access over arrays on top of the `probe_scalar` hook:

- `ArrayProbe<'a>` is a bare `&ArrayRef` with no destructor. One-off
  reads, including every child read while recursing through nested
  arrays, own nothing, so there is no unwind cleanup, no out-of-line drop
  glue and no pinned temporary on the per-row path. This is what the
  CodSpeed regression on the earlier design came down to.
- `RepeatedArrayProbe` owns its handle, encoding state, validity probe
  and child probes, created on first use and reused across rows.
- Both implement `Probe`. `ArrayRef::probe()` and
  `ArrayRef::repeated_probe()` are the entry points; `execute_scalar` and
  `is_valid` are shims over `probe()`.
- `ProbeState<'_, V>` carries the `ArrayView` and, for a repeated read, a
  borrow of the retained `RepeatedState`; it has no destructor.
  `state.array()`, `state.retained()`, `state.slot(i) -> impl Probe`, and
  the direct `child_scalar` / `child_is_valid`. Encodings never branch on
  policy for child reads.
- `Validity::probe()` gives the retained validity accessor.
- `Struct` and `Primitive` implement `probe_scalar`; `scalar_at`
  delegates through `ProbeState::once`.

Not generic over the retention policy: a generic vtable method is
instantiated in every crate that constructs arrays of the encoding, so
the hot body compiles downstream without this crate's inlining; measured
at 20-30 points on struct reads and rejected. The policy is a runtime
`Option` check, folded into the one-off arm by `inline(always)` on the
probe bodies.

Local divan medians vs the develop tip, interleaved runs of 1000
samples: is_valid_per_element[1024] -10%, [256] -12%,
execute_scalar_struct_wide -39%, execute_scalar_struct_simple -29%.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CqrLKgPqFYGZK5sjk1qe7
…tate

- `ArrayProbe<'a>` is now an enum: `Once(&ArrayRef)` for a one-off read,
  `Repeated(&mut RepeatedArrayProbe)` for a borrow of a retained probe.
  Both variants are references, so it still has no destructor, which is
  the property the per-row numbers depend on; a compile-time
  `needs_drop` assertion pins that for `ArrayProbe` and `ProbeState`.
- `ProbeState::slot(i) -> VortexResult<Option<ArrayProbe>>` is the child
  accessor: `None` for an absent slot, an error for an out-of-bounds one.
  `ProbeState::split()` returns the encoding state and a `ProbeChildren`
  as disjoint borrows, so a cache can stay borrowed while children are
  read. The `child_scalar` / `child_is_valid` shortcuts are gone; reading
  `Struct` fields through `slot()` measures within noise of them.
- `ProbeState::once` is crate-private: it exists for the `scalar_at`
  delegation and goes away with `scalar_at`.
- The `Probe` trait and the private `ChildProbe` enum are gone; the enum
  is the one borrowed reader and `RepeatedArrayProbe::as_probe` gives the
  retained variant where a probe is expected.
- `ProbeValidity` and `Validity::probe` are gone: `RepeatedArrayProbe`
  resolves validity once into `uniform_validity: Option<bool>` or a
  boxed validity probe.
- `ProbeStorage` is gone; the erased state slot is an
  `Option<Box<dyn Any>>` with a crate-private `repeated_state` accessor.
- `RepeatedState` is crate-private, and the retained side lives in
  `probe/repeated.rs`.
- No `inline(always)` anywhere: the per-row bodies are plain `#[inline]`.
  That leaves the one-off shim one thin frame above the encoding, which
  measures at parity with develop on `is_valid_per_element`; forcing the
  bodies inline was worth about 10% there and is deliberately not taken.

Public probe items: `ArrayProbe`, `RepeatedArrayProbe`, `ProbeState`,
`ProbeChildren`, and the `EncodingProbeState` alias.

Local divan medians vs the develop tip, five interleaved runs of 1000
samples: is_valid_per_element at parity, execute_scalar_struct_wide
about -34%, execute_scalar_struct_simple about -26%.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016CqrLKgPqFYGZK5sjk1qe7
@joseph-isaacs joseph-isaacs changed the title feat(array): add scalar probes with lazy child contexts feat(array): scalar probes with one-off and repeated probe types Sep 15, 2026
`probe/array.rs` imports `OperationsVTable`, so the label resolves on its own
and the explicit `crate::vtable::...` target duplicates it, tripping
`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
@joseph-isaacs
joseph-isaacs merged commit 976b1ee into develop Sep 15, 2026
80 of 81 checks passed
@joseph-isaacs
joseph-isaacs deleted the ji/array-probe-api branch September 15, 2026 21:14
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.

2 participants