[danking]

Use the logical-row page index to locate scalar probes and cache prepared or decoded pages across probes. This avoids whole-array decoding for sparse access while preserving the existing probe path for files without the index. Cached memory grows with touched pages, and CodSpeed reports retained-probe and scalar-access regressions that remain unresolved.

[codspeed]

Merging this PR will regress 4 benchmarks

4 warnings, 1 note

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

⚠️ Unable to measure the execution of 2 benchmarks

These benchmarks measured no execution time, so their results are not comparable and count as unchanged. This usually happens when the compiler removes the code under test.

Prevent compiler optimizations

See affected benchmarks

⚠️ 19 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

ℹ️ 40 benchmarks have no flamegraph

The flamegraph wasn't generated: the symbols of the profiling trace could not be resolved, because no information about the language frames was collected.

See affected benchmarks

⚡ 6 improved benchmarks
❌ 4 regressed benchmarks
✅ 2455 untouched benchmarks
⏩ 12 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	Walltime	dbp_split_kernel_all_valid_avx2[(I128, 8192)]	7.8 µs	12.1 µs	-36.18%
❌	CPU Simulation	retained_probes[false]	344.4 ms	391.1 ms	-11.95%
❌	CPU Simulation	retained_probes[true]	461.1 ms	514.9 ms	-10.45%
❌	CPU Simulation	scalar_access[(1024, true, false)]	684.8 µs	761.4 µs	-10.05%
⚡	CPU Simulation	first_probes[true]	58,959.5 µs	989.2 µs	×60
⚡	CPU Simulation	first_probes[false]	59 ms	26.2 ms	×2.3
⚡	CPU Simulation	test_scan_filter_to_arrow_array[int_float]	4 ms	3.5 ms	+16.33%
⚡	CPU Simulation	test_repeated_scan_filter[int_float]	3.8 ms	3.4 ms	+12.4%
⚡	CPU Simulation	test_repeated_scan_filter[int]	2.7 ms	2.4 ms	+11.81%
⚡	CPU Simulation	test_scan_filter_to_arrow_array[int]	2.8 ms	2.5 ms	+11.31%

Tip

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

---

_{Comparing dk/pco-indexed-probes (7da4edd) with dk/pco-selective-ranges (26de111)}

Footnotes

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

[danking]

@claude review

[github-actions]

Claude finished @danking's task in 1m 2s —— View job

---

Review: Use PCO logical-page indexes for retained scalar probes

• Read the diff against origin/dk/pco-selective-ranges
• Read access.rs, the probe callers (RepeatedArrayProbe, ArrayProbe::Once) and PcoData::decompress
• Post review

I found no correctness bugs. The refactor removes about 130 lines of duplicated page/rank/decoder code from probe.rs and routes scalar probes through the shared AccessState, which is a good simplification. I traced the hit path, the miss path and the all-null path, and noted a few smaller points below.

Correctness checks that passed

• Bounds: cached_scalar only checks row < unsliced_n_rows, so on its own it would return a cached value for a sliced array when index >= array.len(). This can't happen in practice: RepeatedArrayProbe::execute_scalar and execute_scalar_once both call check_bounds before reaching probe_scalar_retained/probe_scalar_once.
• Logical hit path: a page only reaches last_page with rows: Some(..) after resolve_impl has checked mask.true_count() == page.values.len(). Since decoded has page.values.len() elements, decoded.as_slice::<T>()[value] cannot go out of range.
• Failed decode: if values() fails, last_page keeps decoded: None. cached_scalar then returns None, and the next call takes the miss path and retries. This is correct.
• validated_all_null: it is only set when index.pages.is_empty() and the full mask is all false, so the stored count (0) and the validity agree. Setting it in both resolve_impl and span is consistent.

Points to consider

1. Behaviour change for one-off probes (scalar_at_once), probe.rs:51. On the base branch, scalar_at(.., None, ..) went through _slice(i, i+1).decompress(..). That path ran the full validity mask and checked every logical page's true_count against its stored values (array.rs:714-724). The new path checks only the page it touches. This is faster and matches the PR description ("validating touched page validity before decoding"), but it does weaken error reporting: a stateless scalar_at no longer fails on inconsistent validity in other pages. Please confirm this is intended and that the compatibility tests in the earlier stacked PRs don't rely on the whole-array check through scalar_at.

2. One-off probes build the full rank table, probe.rs:66 → access.rs:resolve. resolve always uses INDEXED = true, so value_index calls rank(), which initialises the stride-512 ranks vector over the whole mask. For a one-off probe, the state is thrown away right after:

   • Logical files: the whole page's rank vector is built for one lookup.
   • Legacy files: the rank vector covers the full array (n/512 words plus a full popcount), on top of the full mask execution that the base branch also did.

   boundary_rank (INDEXED = false) answers a single lookup without allocating. Consider giving scalar_at_once its own resolve that uses INDEXED = false, or making resolve generic over it. This should help the fresh-state "defined/missing" Divan cases the PR description mentions. Fix this →

3. The bounds check in the all-null branch is redundant and inconsistent, probe.rs:35-39. vortex_ensure!(index < array.len()) runs only in the validated_all_null branch. The cached_scalar hit path just below it only checks unsliced bounds. Both callers already run check_bounds, so either:

   • drop the vortex_ensure! (a debug_assert! would do), or
   • if you want defence in depth inside the encoding, apply the same index < array.len() check before cached_scalar.

   The new test retained_all_null_probes_preserve_slice_bounds passes because of the outer check_bounds, not this check.

4. Doc comment on cached_scalar (access.rs:119). It says "last fully validated decoded page". On the legacy route, resolve_impl never checks the mask's total true_count against the stored values; only span does. cached_scalar can't index out of bounds there, because cached.values.contains(&value) guards the lookup. Still, "validated" overstates what the legacy path checks. Either reword the comment, or add the same vortex_ensure_eq!(mask.true_count(), total, ..) to the legacy branch of resolve_impl so resolve and span behave the same.

5. Nits:

   • cached_scalar and validated_all_null are pub fn on a pub(crate) type. pub(crate) would state the intent more clearly, although the other methods use pub too.
   • The PcoProbeState doc says "Construction performs no allocation". That's still true because BTreeMap::new doesn't allocate, but AccessState now also holds the Box<dyn Any> decoder. A short mention that the decoder is retained across probes would help readers.

Tests

• The new all-null sliced test is a good regression.
• I'd also add a sliced test on nullable, non-null data. It should probe a row in the last cached page that sits just outside the slice, and also alternate between null and valid rows across a page boundary. That would exercise the cached_scalar → scalar_at_uncached handoff with the logical index.

Checks

Per the repo's AGENTS.md, I ran no build, clippy or tests. This review comes from reading the code only.