[danking]

Add blackbox tests for PCO selection, slicing, scalar probes, nulls, value ordering, and file compatibility. The tests pass against the existing implementation and establish the public behavior that subsequent page-index optimizations must preserve.

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

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

⚡ 4 improved benchmarks
❌ 3 regressed benchmarks
✅ 2458 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.7 µs	200.6 µs	-26.85%
❌	CPU Simulation	test_scan_filter_to_arrow_array[int_float]	3.5 ms	4 ms	-13.5%
❌	CPU Simulation	test_repeated_scan_filter[int_float]	3.4 ms	3.8 ms	-10.64%
⚡	CPU Simulation	filter_powerlaw_by_random[250000]	174.3 µs	131.6 µs	+32.39%
⚡	Walltime	threshold_avx2[80]	3 µs	2.6 µs	+14.84%
⚡	CPU Simulation	random_i8[0.01]	115.9 µs	101.8 µs	+13.83%
⚡	Walltime	compare_u64_avx2	3.7 µs	3.3 µs	+10.63%

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-selection-contracts (f56bceb) with dk/pco-selection-benchmarks (2736d4c)}

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

---

Review: PCO selection contract tests

• Read the diff against origin/dk/pco-selection-benchmarks
• Check the PCO APIs, dev-dependencies and existing tests
• Post review

The tests are well-scoped and test-only. They cover nested slices, repeated and out-of-order indices, nullable indices, stride-0 and zero-length PiecewiseSequence ranges, long and all-null runs, empty takes, and garbage values under null indices. All three commits have the Signed-off-by line and both new files have SPDX headers. I found no blockers, but some of these tests check less than the PR description says.

1. The expected values are not independent of the code under test (main point)

The PR body says the results are checked against "independent PrimitiveArray results". But PCO has no Take kernel today: encodings/pco/src has only SliceReduce and a probe. So compressed.take(idx) decompresses and then runs the same primitive take kernel that builds the expected value in input.take(idx). Right now a test like invalid_indices_under_nulls_are_ignored mostly checks that primitive take agrees with itself. If primitive take mishandled i64::MIN under a null index, both sides would be wrong in the same way, or both would error and the test would just fail without saying which side broke. AGENTS.md asks for expected results taken from the input contract.

The protection only kicks in once a later PR adds a PCO-specific take path. That is the stated goal, so it's fine, but the small cases are cheap to pin down as literals:

// invalid_indices_under_nulls_are_ignored
assert_arrays_eq!(
    compressed.take(indices)?,
    PrimitiveArray::from_option_iter([Some(11i32), None, None, Some(33)]),
    &mut ctx
);

The same applies to the 8-element take in selected_values_and_nulls and the 5-element take in null_runs_and_empty. Those expected values are easy to write as from_option_iter literals. compact_ranges is less affected because its oracle uses a different index encoding, and pco_validity_selection.rs already builds expected independently.

Fix this →

2. No test crosses a PCO chunk boundary

Pco::from_primitive always uses VALUES_PER_CHUNK = pco::DEFAULT_MAX_PAGE_N (2^18 = 262,144). The largest input here is 131,072 rows, so every test fits in one chunk; only page boundaries are covered (pages of 31, 7, 128, 127 and 2 values). Chunk selection is an obvious place for an optimized take or probe to go wrong, for example in the index from a selection to its chunk. Two ways to cover it:

• Use one input just over 262,144 rows, with indices and a slice spanning the boundary. This stays within the public-API-only rule.
• Or use PcoData::from_primitive_with_values_per_chunk, which is pub(crate) and so reachable from this in-crate module, with a small chunk size. That is cheaper but no longer black-box.

3. Smaller points

• Inconsistent session setup: only selected_values_and_nulls calls session.arrays().register(Pco) (selection_contract_tests.rs:22). The other three tests pass without it. Either drop it or register in all tests (a small fn ctx() helper would also remove four copies of the setup).
• Test placement: vortex-file/tests/pco_validity_selection.rs does no file IO. It only uses Pco, RunEnd and register_default_encodings. It probably lives there because vortex-runend isn't a vortex-pco dev-dependency. Adding vortex-runend as a dev-dependency of vortex-pco would keep all PCO contracts in one place and avoid adding an integration-test binary to vortex-file. Not a blocker; if it stays, a one-line doc comment explaining why would help.
• Out-of-bounds probe: assert!(probe.execute_scalar(source.len(), ..).is_err()) appears only in the first test. Commit d9825fb says out-of-bounds take rejection was moved to the later guard PR, so this is fine. Just confirm that probe out-of-bounds behaviour is meant to be fixed now, unlike take.
• Nit: in null_runs_and_empty, PrimitiveArray::from_iter([] as [u64; 0]).into_array() is built twice; bind it once and clone it, as the other tests do with indices.
• Nit: selection_contract_tests.rs is a second #[cfg(test)] module next to tests.rs. AGENTS.md prefers a tests/ directory module (tests/mod.rs + tests/selection.rs) once tests grow.

CodSpeed

The reported changes are missing_stats[16] in vortex-layout and a Python scan benchmark. This PR only adds #[cfg(test)] code and an integration test, so neither can be affected. CodSpeed itself warns that base and head ran on different runner types; this is noise.

Checks

I did not run any builds or tests. Following AGENTS.md, verification is left to you. The narrow commands would be cargo nextest run -p vortex-pco and cargo nextest run -p vortex-file --test pco_validity_selection.

[danking]

@claude review

[github-actions]

Claude finished @danking's task in 54s —— View job

---

Review: PCO selection contract tests (second pass, through 39dadb2)

• Read the diff against origin/dk/pco-selection-benchmarks (6 commits)
• Check every literal expected value by hand
• Check how PcoData splits values into chunks
• Post review

The new commits fix the two main points from my last review. The expected values are now written out as literals, and there is a test that crosses a chunk boundary. I checked every hand-written expected value and found no errors. All 6 commits have Signed-off-by, and both new files have SPDX headers. I found no blockers. One coverage gap is still worth closing before the later optimization PR.

Expected values I checked

• selected_values_and_nulls: the two nested slices start at original row 30. So 3000 → 3030·19 = 57570, 127 → 157·19 = 2983, 128 → 3002. Index 5 maps to row 35, and 35 % 7 == 0, so it is null only when nullable is true. All other selected rows are valid.
• null_runs_and_empty: rows 0 and 65001 are outside the null run 100..65000 and row 30000 is inside it. The expected values match.
• nonnullable_selection_crosses_chunks: 299999 → 899998, 262144 → 786433, 262143 → 786430. All correct, and 262143/262144 are on the two sides of the 2^18 chunk boundary.
• pco_validity_selection.rs: RunEnd ends are exclusive, so rows in [32768, 65536) and [98304, 131072) are valid. The literal take result matches. The slice and probe oracle (expected) is built from a formula, not by running RunEnd.
• compact_ranges: the expected indices are worked out directly from (start, len, stride), so the check does not depend on how PiecewiseSequence is executed.

1. No nullable input crosses a chunk boundary (main remaining gap)

PcoData::from_primitive_with_values_per_chunk calls collect_valid and splits the dense valid values into chunks (array.rs:534-545). Chunk boundaries therefore come from counting valid values, not from logical row numbers. Turning a logical index into "chunk N, offset M" is the step an optimized take or probe is most likely to get wrong. The only chunk-crossing test uses a non-nullable input, where logical index and dense index are the same, so it cannot catch that mistake.

Suggested test: a nullable input of about 400k rows with roughly ⅓ nulls, so the dense boundary lands at a logical row that is not a round number. Select indices on both sides of the row whose dense index is 262 143 / 262 144, and add a slice and a few probes across that row. The expected values stay easy to write by hand if values and validity are both formulas of i, as in compact_ranges.

Fix this →

2. Smaller points

• Slice and probe across the chunk boundary: nonnullable_selection_crosses_chunks covers only take. Slicing at 262000..262300, then taking or probing inside the slice, would also cover the chunk-skip logic in slice (array.rs:~655). This can go into the same new test as point 1.
• register(Pco) in only one test: this is still unchanged from my last review. Only selected_values_and_nulls calls session.arrays().register(Pco) (selection_contract_tests.rs:22). Either remove it, or move the setup into a small fn ctx() helper that all tests use.
• Test placement: vortex-file/tests/pco_validity_selection.rs still does no file IO. It needs vortex-runend, which is a dependency of vortex-file but not a dev-dependency of vortex-pco. Adding vortex-runend as a dev-dependency of vortex-pco would keep all PCO selection tests in one crate. If you keep it in vortex-file, add a one-line comment saying why.
• Nit: .then(|| { i64::try_from(i * i).unwrap() }) in compact_ranges has redundant braces inside the closure. rustfmt keeps them, so you'd have to remove them by hand.

CodSpeed

The PR adds only #[cfg(test)] code and an integration test, so it cannot change benchmark results. The reported changes are in the Python scan and filter benchmarks, and CodSpeed itself warns that base and head ran on different runner types. I'd treat this as noise.

Checks

I didn't run any builds or tests; AGENTS.md leaves that to you. The narrowest checks are cargo nextest run -p vortex-pco and cargo nextest run -p vortex-file --test pco_validity_selection.