[danking]

Execute selective primitive Take for arrays with the new logical-row prefix. Locate touched pages, prepare their local validity, reuse PCO file/chunk decoder setup, call the existing native PrimitiveTake for decoded-page groups, and scatter requested values in their original order with repetitions and logical nulls preserved. Contiguous primitive requests use bulk spans; broad requests retain canonical fallback.

A missing prefix immediately returns the existing Take fallback before preparation. The indexed helper has only an O(chunks) page map and one operation-owned prepared page; it does not implement an old-format selective reader or a generic indexed/legacy strategy framework. New decode/output buffers are initialized. Public argument bounds/overflow contracts are separate from file-content consistency checks.

Fourth replacement for #10771, after #10827. All semantic tests are in #10825, with public initialization wiring here. All 63 PCO library tests and all-target/all-feature Clippy pass. Fresh current-head public file compatibility passed both directions against unchanged base33675. The final new-only matched resource run completed with all 26 oracles passing. 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 sparse Take medians were 0.6009/0.5619 ms versus 41.90/71.27 ms original baseline (dense/nullable, about 70×/127×); contiguous Take was 1.61×/2.13× faster.

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.

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 3 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 1 benchmark

This benchmark measured no execution time, so its result is not comparable and counts as unchanged. This usually happens when the compiler removes the code under test.

Prevent compiler optimizations

See affected benchmarks

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

⚡ 5 improved benchmarks
❌ 3 regressed benchmarks
✅ 2457 untouched benchmarks
⏩ 12 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	CPU Simulation	chunked_dict_primitive_canonical_into[f64, (1000, 1000, 10)]	146.5 µs	200.7 µs	-27.02%
❌	CPU Simulation	metadata_open_batch[true]	506.8 ms	646.6 ms	-21.62%
❌	CPU Simulation	test_scan_filter[int]	3 ms	3.4 ms	-11.22%
⚡	CPU Simulation	selection[("ranges", true)]	540.1 ms	2.1 ms	×260
⚡	CPU Simulation	selection[("ranges", false)]	344.7 ms	1.7 ms	×200
⚡	CPU Simulation	selection[("sparse", true)]	539.7 ms	10.9 ms	×50
⚡	CPU Simulation	selection[("sparse", false)]	344.4 ms	12.2 ms	×28
⚡	CPU Simulation	selection[("contiguous", true)]	638.3 ms	516.8 ms	+23.51%

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-selective-take (eea0ae2) with dk/pco-logical-page-index (0d82a2d)}

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 2m 28s —— View job

---

PR #10828 review: selective PCO take

• Gather context (diff vs dk/pco-logical-page-index)
• Review take.rs, access.rs, decoder.rs
• Review tests and registration call sites
• Post findings

The sparse/grouped design holds up. Requests are sorted, repeated rows are resolved once, each page is decoded once, and results are scattered into an initialized output. Bounds checks run before any allocation, and legacy metadata uses a single ranked validity mask instead of a per-index scan. The kernel registration matches how ALP, BitPacked and the others do it. I found one test-module compile error that will break CI, and one large block of code that can never run.

🔴 Blocking

1. selective_take_tests.rs doesn't compile: PiecewiseSequenceArray is not imported.
contiguous_take_preserves_nullable_index_dtype (L257) calls PiecewiseSequenceArray::try_new(...), but the import list (L4–23) has no use vortex_array::arrays::PiecewiseSequenceArray;. selection_contract_tests.rs does import it, but that import doesn't reach this module. Because the module won't build, every vortex-pco test fails, not just this one. I didn't compile this locally; CI "Linters and Tests" was still queued when I checked.
Fix this →

🟠 Should address

2. gather_typed, the Output<T, NULLABLE> helper and the stride-0/strided handling can never run (about 200 lines in take.rs L433–605).
Here is how TakeExecute::take routes requests:

• grouped = (!summary.contiguous).then_some(..), and contiguous can only be true when the mask is AllTrue and every index is the previous one plus 1 (L155–163).
• So whenever grouped is None, primitive_runs returns exactly one Run { start: Some(_), stride: 1 }. gather pattern-matches that case and goes straight to stream_range (L325–334).
• Therefore gather_typed::<_, true/false> is never reached. The same goes for the run.stride == 0 branch, the strided for offset loop, and the zero-page branch in gather (L335–348), which zero_page_take already handles at L91.

Run.stride is always 1 in this PR. My guess is this code is staged for the PiecewiseSequence follow-up. If so, I'd move it to that PR, where it will actually be called and tested. Otherwise it ships untested, and the compiler won't flag it because it is technically reachable. At minimum, add a comment saying it's reserved for range descriptors.

3. The CodSpeed regression in test_repeated_scan_scalar_at[int] (−14%) needs a route check before it's dismissed.
Base and head ran on different runner types, so this may be noise. Still, every single-index take with a valid index now goes through the following steps:

• execute::<PrimitiveArray>
• min_max
• execute_mask
• stream_range, which builds a fresh PageDecoder, computes the whole-page validity mask, and decodes the full page into scratch

If scalar-at scans end up in this kernel, that fixed overhead per call is a plausible cause. A cheap early exit for indices.len() == 1 (resolve plus a single page read) might be worth it, or confirm that this path isn't hit.

4. The module docs still show the old registration.
lib.rs L12–20 tells users to call session.arrays().register(vortex_pco::Pco). That still works, but it silently skips the take kernel, as your own registry_only_session_retains_legacy_take_fallback test shows. Point the example at vortex_pco::initialize(&session). Related: vortex-btrblocks/tests/golden.rs:456 and the existing pco test sessions (tests.rs, page_index_tests.rs, selection_contract_tests.rs) register only Pco, so they never exercise the new kernel. That's probably intentional for some of them, but worth a look.

🟡 Minor / style

5. Imports inside initialize (lib.rs L88–92) are on the CLAUDE.md "Common Mistakes" list. Move them to module level, or follow ALP's split and put initialize in rules.rs / kernel.rs. Likewise, access.rs L393/L395 spells out crate::decoder::PageDecoder when take.rs already imports it.
6. Validation differs between legacy paths. In access.rs, span checks that mask.true_count() equals the total stored values (L278–282). resolve_impl (legacy branch, L231–236) doesn't. If the validity marks fewer valid rows than there are stored values, the grouped path quietly returns misaligned values while the streaming path errors. I'd run the same vortex_ensure_eq! when self.legacy is first populated.
7. AccessState::values indexes array.chunk_metas[page.chunk] directly (L397). Corrupt metadata would panic there, while stream_range uses .get(..).ok_or_else(..). Using .get here too would keep it consistent.
8. gather_grouped keeps every decoded page alive until the end because it uses AccessState::<true>. Pages are visited in sorted order and never revisited, so retaining them only adds peak memory, roughly the size of all touched pages. prefer_canonical_primitive caps that below the canonical size, but dropping decoded after each page group (or using a non-retaining decode for the values phase) would bring it down to one page.
9. PageDecoder::read zero-fills the whole buffer and then decodes over it (decoder.rs L46). That's harmless but wastes one pass per decoded page in the grouped path. Use with_capacity plus decoding into spare capacity if pco allows it, or just note why the zero-fill is needed.
10. Legacy heuristic in prefer_canonical_primitive. requests >= index.pages.len() means a legacy array with only a few pages always takes the canonical path. That's reasonable, but a short comment explaining why the page count stands in for the byte estimate would help reviewers.
11. validate_runs repeats the bounds check. validate_extrema already proved every defined index is within bounds. I'd keep it only if it exists for the upcoming range descriptors (see item 2).

✅ Looks good

• Adding Page::chunk to the page index, together with a per-operation BTreeMap<usize, ChunkDecompressor>, cleanly fixes multi-chunk addressing (commit 4c93a25).
• read_into checks both n_processed and finished, so a truncated page can never write a partially filled output.
• Null indices with out-of-range physical values (i64::MIN / i64::MAX) are never inspected, and there's a test for it.
• stream_range copies the sliced source validity directly instead of rebuilding it.
• Test coverage is good: sliced-of-sliced sources, legacy and unknown metadata versions, repeated and scattered requests, empty and mostly-null requests, and dtype nullability.

Checks

I did not run any builds or tests; this was a read-only review. Item 1 comes from reading the import list, not from a compile. CI "Linters and Tests" was still queued when I checked.

[danking]

@claude review

[github-actions]

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

---

PR #10828 review: selective PCO take (re-review at 7013dfc)

• Gather context (diff vs dk/pco-logical-page-index)
• Review take.rs, access.rs, page_index.rs, decoder.rs
• Review tests and registration call sites
• Post findings

This revision fixes most of what the last review raised:

• The PiecewiseSequenceArray import is back.
• The unreachable gather_typed / Output code is gone.
• initialize now uses module-level imports.
• gather_grouped drops AccessState before decoding and keeps only one decoded page at a time.

The PageIndex::{IndexedFast, LegacySlow} split is clean. The mode is picked once at operation entry, Mode is a type parameter inside the loops, and only legacy files build global value prefixes. One robustness issue remains, plus a few smaller items.

🟠 Should address

1. A malformed logical_row_ends panics instead of returning an error (page_index.rs L69/L72, access.rs L259)

Nothing checks logical_row_prefix. PcoData::validate (array.rs L431–492) and deserialize both ignore it, and PageIndex::try_new only tests is_some(). The PR description says skipping this check is deliberate. However, the new code indexes into the vector directly, so a corrupt or hostile file turns into a panic rather than a VortexError:

• Prefix too short or last end too small: say the last entry is less than unsliced_n_rows, or the list has fewer entries than there are pages. find_row(row) then returns row_ends.len(), and IndexedView::rows panics on self.row_ends[page].
• Ends not monotonic: row - location.rows.start in Indexed::resolve / span can underflow. That panics in debug builds and wraps in release, after which bits.value(huge) panics.

Every other metadata access on this path already uses .get(..).ok_or_else(..), so this is the one gap. The cheapest fix is a single O(pages) check in PageIndex::try_new, which is cached in the OnceLock and so runs once per array owner. It should confirm that:

• row_ends.len() == page_count
• the ends never decrease
• the last end equals unsliced_n_rows (pass it in)

That check doesn't compare against validity, so it still fits the PR's "no page-index/validity consistency validation" stance. If you'd rather keep try_new free of checks, switch rows() to .get() and use checked_sub instead.
Fix this →

2. prefer_canonical_primitive does a BTreeSet insert for every valid index (take.rs L235–249)

For large sparse requests (for example 1M indices), the planning pass runs find_row plus a BTreeSet::insert per index before any decoding starts. resolve later repeats the same page search. Indices that land on the same page are usually adjacent, so checking against the last page touched before the set lookup would skip most inserts. Alternatively, since touched only feeds a byte estimate, a Vec<bool> sized to the page count works too (it is O(pages), which is already the scale of the metadata).

3. gather_grouped runs the generic take once per page to pull out a few values (take.rs L368–397)

For each page group the code:

• collects a Vec<u64>
• wraps it in a PrimitiveArray
• dispatches take
• calls execute::<PrimitiveArray>

All of that just to read decoded[value]. decoded is already a non-nullable PrimitiveArray<T>, so reading decoded.as_slice::<T>().get(value).ok_or_else(..) directly inside the scatter loop is simpler, avoids two allocations and a kernel dispatch per page, and still errors on an out-of-range value index.

🟡 Minor

4. Stale snapshot guard in register_default_encodings (vortex-file/src/lib.rs L180–181). let arrays = session.arrays(); is held across initialize_pco(session), which re-reads arrays() and writes kernels(). It works because the registries mutate in place. Still, the SessionGuard docs warn that holding the guard pins an arc-swap slot while writes happen. Calling initialize_pco(session) alongside the other initialize calls, outside the block, keeps it consistent.
5. Module docs still show session.arrays().register(Pco) (lib.rs L12–20). Your own registry_only_session_retains_legacy_take_fallback test shows that path silently skips the kernel. Point the docs at vortex_pco::initialize(&session). tests.rs:35 and vortex-btrblocks/tests/golden.rs:456 also register only Pco. That's probably fine, but those suites never exercise the new kernel.
6. stream_range_mode's view.page_count() == 0 branch can't run (take.rs L431–437). zero_page_take already returns for array.pages.is_empty() before gather is called.
7. Run::stride is always 1 in this PR, so offset * run.stride and the stride: 1 pattern in gather exist only for the PiecewiseSequence follow-up. A one-line comment saying so would help reviewers.
8. A single valid index still decodes a full page. Any length-1 take counts as contiguous because windows(2) is empty, so it goes through stream_range_mode: a new PageDecoder, the page mask, and a full-page decode. This is correct, and scalar probing doesn't use this path. It's only worth an early exit if single-row takes turn out to matter.
9. CodSpeed metadata_open_batch[true] −17%. The timed closure only calls ArrayPlugin::deserialize, and this PR doesn't touch deserialize apart from adding an empty Arc<OnceLock> to PcoData. Base and head ran on different runner types, so I'd treat it as noise unless it reproduces. The same goes for the unrelated chunked_dict_primitive_canonical_into result.

✅ Looks good

• Ordering: requests are sorted by row and grouped first by row, then by page. Rows map to pages monotonically in both modes, so partition_point grouping is sound and repeated rows are resolved once.
• Coordinates: row ends are kept unsliced and slice_start is added once when requests are built. The sliced-of-sliced test covers both formats.
• Legacy page spans: Legacy::span finds the logical end of a page with select on the sliced bitmap and handles all-null gaps with a zero-value span, which stream_range_mode skips.
• Decoder setup: PageDecoder reuses FileDecompressor and one ChunkDecompressor per chunk for the whole operation. Codec errors propagate.
• Fallbacks and bounds: min/max bounds checks never look at null physical index values, and both fallbacks (descriptor-byte and touched-page) return None before anything is allocated.

Checks

I didn't run any builds or tests; this was a read-only review. CI status wasn't checked for this revision.