[danking]

Use an operation-owned scalar-probe cache only when the new logical-row prefix is present. Bind the primitive decoder once, resolve requested values through page-local validity, retain touched prepared/decoded pages, and return direct last-page hits. The existing missing-prefix probe body and fields are preserved, with only the optional indexed state and dispatch branch added.

Retained state can grow to the full touched decoded array; no fixed memory-budget claim is made. Fresh-state defined and missing probes on a prepared owner and retained warm probes are separate workloads. They exclude file opening, and their different states do not justify a cold/warm break-even inference.

Sixth replacement for #10771, after #10829. All semantic tests originate in #10825. All 63 PCO library tests and all-target/all-feature Clippy pass. Historical pre-simplification results are retained. Fresh current-head public file compatibility passed both directions against unchanged base33675, and all 26 matched resource oracles passed. Current GitHub CI and human review remain separate gates.

Existing CI and CodSpeed are reused; no new workflow. AI-assisted development with OpenAI Codex. Normal CI and the required human reviews remain necessary before merge; this PR remains draft.

Measured on an AMD EPYC 7B13 cloud VM: three OS-timer samples within one process per version, fixed workloads, resident buffers, no IO measurement. These are point estimates, not ABBA or confidence intervals; whole-process CPU/RSS includes fixtures and oracles. Final-stack comparisons do not isolate individual commits.

Explicit final-stack tradeoffs: nullable repeated Take rose from 52.43 to 67.71 ms (+29.1%, +15.28 ms); metadata-open/drop batches rose +33.2% dense/+31.6% nullable. These adverse results are retained for maintainer judgment, not hidden or auto-tuned away. No overall weighted speedup is claimed.

Final-stack prepared-owner fresh-defined batches (256 probes) measured 13.82→1.315 ms (10.5×), and fresh-missing batches 14.17→0.1026 ms (138×). The latter is a missing-value result, not defined-value decompression. Retained batches (1,048,576 probes) measured 47.56→46.50 ms dense and 53.38→50.68 ms nullable: near parity, with only small point-estimate differences.

CI correction: the existing compact stock-ticker expected size was updated 55,344→55,396 (+52 bytes with optional metadata), preserving its semantic roundtrip and default cases. The indexed-slice brace received pinned formatting. Fresh standard CI runs passed the source, platform, formatting and TSAN checks for these minimal corrections; no unrelated decoder fix is bundled.

Upstream CodSpeed also reports real independent-model regressions for retained probes (about 12–14%) and an existing 1,024-row scalar_access case (about 11%). That original case is not directly covered by the locked 26 cloud controls. These signals remain outstanding for maintainer review; they are not reclassified as infrastructure or equated with OS wall time. The earlier SVE toolchain-network failure was superseded by a successful fresh standard run.

Final CI audit: source/platform/formatting/TSAN checks have passed; remaining performance gates are CodSpeed and required human review. All PRs remain draft; no performance waiver or merge approval is claimed.

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

[danking]

@claude review

[github-actions]

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

---

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

• Read the diff against origin/dk/pco-selective-ranges (7 commits, up to 969c7c6)
• Read access.rs, decoder.rs, page_index.rs and the probe tests
• Post review

This revision addresses the earlier review. One-off probes now call resolve::<false>, so they use boundary_rank and skip the stride-512 rank table. The duplicated bounds check in the all-null branch is gone. The binding design holds up: an Arc owner plus a typed TypedProbe<T, P> behind one Box<dyn BoundProbe>, with a hit path that is just cached_position + last_values. I found no correctness bugs for valid inputs. The main gaps are corrupt-input handling and test coverage.

Traced and correct

• Hit/miss sync: two "last" slots move independently, AccessState.cache.last and DecodedPages.last. They can disagree in two cases: after a null probe on the indexed path, which moves cache.last without decoding, or after a failed decode. Both are safe. last_values(page) returns None, the probe takes the slow path, and resolve + values bring the two slots back in line.
• Legacy null probes return Some(None) from cached_position before looking at the cache, so null rows never reach resolve or the decoder. Good.
• DecodedPages::values rewrite: pages are swapped between last and the BTreeMap, and nothing is dropped. The range/take callers keep their retain-everything behaviour.
• Zero-page early return (probe.rs, scalar_at): this is safe. A non-nullable array with no pages has length 0, so check_bounds rejects every index before scalar_at runs.

Points to consider

1. Corrupt validity can panic on indexed files: probe.rs, both values.as_slice::<T>()[value] sites. value comes from the page's own validity bits (rank/boundary_rank of row - rows.start). Nothing checks it against n_values for the page. The PR description says this is deliberate ("without comparing whole-page popcounts against metadata"). The consequence is that a page whose validity has more set bits than stored values triggers a slice-index panic, not a VortexResult error. A per-probe bounds check fixes this cheaply without bringing back the whole-page popcount:

   let v = values.as_slice::<T>().get(value).copied()
       .ok_or_else(|| vortex_err!("Pco validity selects a value beyond the decoded page"))?;

   The legacy path has a similar older problem that this PR does not introduce. When value >= total, find_value returns value_ends.len() and LegacyView::values(page) panics on value_ends[page]. If you add the guard above, consider also guarding page < value_ends.len() there.
   Fix this →

2. Retained probes on legacy files have no tests. ScalarMode for Legacy::cached_position is new code. It is the only place that turns a global dense ordinal into a within-page offset on the hit path (value - values.start), and probe/tests.rs only builds indexed (current-format) arrays. selective_take_tests.rs and page_index_tests.rs already have a legacy(&ArrayRef) helper. Adding a #[values(false, true)] legacy_format axis to random_access would cover all of the following: legacy × nullable × sliced × repeated, plus the null-hit Some(None) shortcut and alternation across page boundaries.

3. Legacy misses compute the rank twice: access.rs, Legacy::cached_position → Legacy::resolve. On a miss, cached_position computes state.value_index::<true>(row), finds no matching last page, and returns None. resolve then computes the same value_index again. The first call also builds the full-array rank table before cache.last exists. Each rank costs at most one stride popcount, so this is minor. You could avoid it by returning the computed value on a miss, or by having resolve accept a precomputed value.

4. Probe state is bound to the first array it sees. The probe stores the Arc owner, the validity-derived AccessState and the ChunkCursor/decoders from the first call, and reuses them for every later array. The base branch made the same assumption for validity and pages, so nothing changes in practice. However, mixing these up now gives silently wrong values rather than a recomputation. A short line in the PcoProbeState doc, or a debug_assert! that Arc::ptr_eqs the metadata, would make the invariant explicit.

5. Nits:

   • AccessState::cached_position returns Option<Option<(usize, usize)>>. A small enum such as Hit::{Null, Value(page, off)} or Miss would read more clearly at the match in probe.rs.
   • ScalarMode::cached_position is generic over RETAIN, but scalar probes only ever use RETAIN = true. It's harmless, but you could fix it to true.
   • The PcoProbeState doc could say that per-page RankMasks, with their rank tables, are retained too, not only decoded pages. Together they grow to about the full validity bitmap.

Benchmarks

CodSpeed reports first_probes ×58 (true) and ×2.3 (false). That matches the move to per-page masks with boundary rank. The test_scan_filter[int] −11.3% regression is in a Python scan-filter benchmark that doesn't obviously go through scalar probes. CodSpeed also notes that base and head ran on different runner types, so it is probably noise, but it should be checked against a matched rerun before you acknowledge it.

Checks

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