[danking]

Store optional cumulative logical-row page boundaries so readers can locate compressed pages without scanning validity across the array. Existing files remain readable and older readers ignore the additive metadata; shared metadata and buffer vectors avoid copying them between views. Earlier stack measurements showed about 32–33% higher metadata-open/drop time; the latest copy removal has not been rebenchmarked.

[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

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

⚡ 3 improved benchmarks
❌ 3 regressed benchmarks
✅ 2459 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	metadata_open_batch[false]	385.4 ms	588.7 ms	-34.54%
❌	CPU Simulation	metadata_open_batch[true]	346.7 ms	506.8 ms	-31.58%
❌	CPU Simulation	filter_powerlaw_by_random[250000]	131.6 µs	174.7 µs	-24.65%
⚡	CPU Simulation	first_probes[false]	121.8 ms	59 ms	×2.1
⚡	CPU Simulation	first_probes[true]	121.7 ms	59 ms	×2.1
⚡	CPU Simulation	chunked_dict_primitive_canonical_into[f64, (1000, 1000, 10)]	200.6 µs	146.5 µs	+36.93%

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-logical-page-index (0d82a2d) with dk/pco-selection-contracts (f56bceb)}

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

---

Review: optional logical-page boundaries in PCO metadata

• Read the PR diff against dk/pco-selection-contracts
• Check the writer, PageIndex, validation, serialization, cast and probe paths
• Compare against the CodSpeed regressions
• Post the review

Overall the change is sound. The wire format is additive (tag 3, optional, versioned), so older readers that ignore the field still decode correctly. PageIndex::try_new checks the page count, strictly increasing ends, ends - start >= n_values, and full row coverage, and those checks look complete. The issues worth looking at before merge are mostly about cost, plus one behaviour change.

1. Hot-path cost (likely behind the CodSpeed regressions)

a. Every full decode now checks validity counts for every page (array.rs decompress, ~L707–720).

• For each logical page it runs mask.slice(rows).true_count() over the whole unsliced array, even when the array is a 1-row slice.
• scalar_at without probe state calls ._slice(index, index + 1).decompress(...) (probe.rs:60-64). That makes every unprobed scalar read do one mask slice plus one popcount per page across the entire array.
• Mask::slice on Mask::Values may also allocate once per page.
• Possible fixes:
  • Only check the pages the decode actually touches.
  • Or walk the bit buffer once with a running count_range and drop the per-page mask.slice.
  • Or do the check once per owner and cache the result next to the OnceLock. The current comment explicitly avoids caching validity, but a single verified flag is not decoded data.

b. The index is built eagerly when an array is opened. validate() ends with self.page_index()?, so the OnceLock laziness never applies.

• Every deserialize now allocates a Vec<Page> (each Page is a Range plus an Option<Range>) and repeats the per-page loop that validate has just run (empty page check, prefix overflow).
• That fits the roughly 40% regression in metadata_open_batch[true/false]. Each open also pays three extra Arc allocations (chunk_metas, pages, metadata) plus the Arc<OnceLock>.
• Options:
  • Run only the cheap structural check on metadata.logical_pages inside validate's existing loop, and build the Vec<Page> lazily on first use.
  • Or accept the cost and record it explicitly. The PR body already says open overhead needs review.

c. first_probes is about 4× slower, which is unexplained. The probe-state path (probe.rs) builds its own pages vector and never uses page_index(), so the regression probably comes from construction or validation. It's worth profiling before you acknowledge it on CodSpeed. Note that CodSpeed also flags different runner types for base and head.

2. Serializing a sliced array is now a hard error (array.rs ~L219)

The old behaviour was already wrong: it wrote the full metadata with the sliced len, so the result failed validation or was corrupt. Failing loudly is an improvement. However, any path that serializes a sliced Pco without executing it first now fails at write time instead of being normalized. Examples:

• Rewriting scanned data where the row range sliced a flat layout's Pco.
• IPC.
• A layout strategy that slices chunks after compression.

Please confirm that no in-tree writer or strategy (for example vortex-layout repartition, or compressor output that is later split) can hand a sliced Pco to serialize. If any can, it needs to be normalized upstream. Also, the slice_start / slice_stop part of the error condition is fine. The error message could name the slice range to make debugging easier.

3. Correctness nits

• Writer Mask::Values branch (~L611): page.n_values as usize - 1 relies on pco never emitting empty pages. That's true today and is now checked on read, but a checked_sub / vortex_ensure! here would make the writer independent of that assumption.
• No direct test of the ends values. No unit test asserts the emitted ends for leading nulls, trailing nulls, or a page boundary on a null run. Correctness is only implied by the full-decode count check passing.
  • A small rstest would help: decode the wire metadata and compare ends with a naive prefix computation.
  • Cases: all-valid, leading nulls, trailing nulls, and multiple chunks.
• Test name overstates what is checked. checked_metadata_and_touched_page_validity exercises the malformed index through take. Since this PR doesn't install selective take, that goes through the full decode and checks all pages, not just the touched ones. Either rename it or narrow the comment.
• Test for pages[0].n_values = DEFAULT_MAX_PAGE_N + 1. It does reach the intended chunk-size guard, because that check runs before the value-count and index checks in validate. But assert!(is_err()) doesn't pin that down. Matching on the error message would keep it targeted, as AGENTS.md asks ("Invalid-input fixtures should reach the specific guard they claim to test").

4. Equality, hashing and cast

• Hash/eq strength. ArrayEq now compares the whole PcoMetadata, and ArrayHash hashes the header, page counts and ends. Two otherwise identical arrays, one with the index and one without (legacy), now compare unequal. That's probably intended, but call it out if any dedup or caching relies on Precision::Value equality across file versions. The hash could also short-circuit with Arc::ptr_eq on metadata.
• cast.rs. array.data().clone() correctly keeps the slice bounds and the shared OnceLock. Good simplification.

5. Style

• lib.rs doctest. use vortex_array::session::ArraySessionExt; moved below the let, which looks like an accidental reorder and reads oddly. Please restore it to the top.
• Unqualified mask paths. vortex_mask::Mask is used fully qualified in several places in array.rs. A module-level use vortex_mask::Mask; would match STYLE.md.

Summary

The format design and its validation are solid, and the compat harness and absent/unknown-version tests are good.

Before merge:

• Bring the per-decode validity check down to the pages actually decoded, or check it only once per owner.
• Decide whether building the index eagerly at open is acceptable; it costs about 40% on metadata_open_batch.
• Explain the 4× first_probes regression.
• Confirm that no in-tree writer serializes sliced Pco.

I didn't build or run any tests or benchmarks; this review comes from reading the code only.

[danking]

@claude review

[github-actions]

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

---

Review: optional logical-row ends in PCO metadata (head b31ed89)

• Read the diff against origin/dk/pco-selection-contracts
• Check the writer's ends computation, wire format, hash/eq, cast and tests
• Compare with the CodSpeed results
• Post the review

Most of my earlier review was about an older revision: the eager PageIndex, per-decode validity checks, the sliced-serialize guard and wire versioning. All of that is gone now. In this revision the change is small and the format addition is safe. Older readers skip proto tag 3, and nothing reads logical_row_ends yet. My main concern is that the writer now persists data into files with no test checking that it is correct.

1. No test checks the ends the writer emits (most important)

logical_row_ends goes into files today, and PR10828+ will rely on it. If the writer is wrong, the files it writes will carry a bad index for good. The two new tests only cover the case where the index is overwritten (ends[0] = 5) or removed. Nothing checks what from_primitive actually writes.

I traced the logic and it looks right:

• Pages cover (end[i-1], end[i]], so leading nulls belong to the next page and the last page takes the trailing nulls.
• The last-page shortcut works because page_buffers counts pages across all chunks.

I'd still pin it down. Add an rstest that decodes the wire metadata and compares ends with a simple prefix walk over validity. Cases:

• All valid (Mask::AllTrue)
• All null (ends is empty)
• Empty array
• Leading nulls
• Trailing nulls
• A page boundary that falls inside a null run
• More than one chunk (values_per_chunk smaller than n_values)

Fix this →

2. A validation test was deleted

test_validate_rejects_oversized_chunk was removed, apparently only because metadata is now an Arc and can't be changed in place. That leaves the chunk-size guard in validate untested. You can restore it with Arc::make_mut(&mut data.metadata).chunks[0].pages[0].n_values = …. Matching on the error message would also confirm the test reaches that specific guard (see AGENTS.md).

3. Open-path cost (metadata_open_batch −32% to −35%)

The fixture uses 1024 values per page, so it has many pages. Each open now pays for:

• Decoding one packed varint u64 per page for the new field.
• Two extra allocations and copies. deserialize collects into Vec<ByteBuffer>, then new_unchecked converts with .into() to Arc<[ByteBuffer]>, which allocates again and copies.
• An extra Arc::new(metadata).

The varint decode is the price of the feature. The Vec→Arc<[_]> copies are avoidable: collect straight into Arc<[_]>, or keep Vec if the Arc is only there for cheap clones. That is probably also where the 2× first_probes gain comes from (data clones are cheaper). Please say which trade-off you want before acknowledging the regression on CodSpeed.

4. Smaller points

• Writer underflow (array.rs ~L614): page.n_values as usize - 1 assumes pco never emits an empty page. In debug builds an empty page panics. In release it wraps to select(usize::MAX), which returns None, so you get a misleading "too few valid rows" error. A checked_sub with its own error would be cleaner.
• Proto tag numbering: logical_row_ends uses tag 2 inside PcoLogicalRowPrefix, and tag 1 is unused. I assume tag 1 was the removed version field. If any development fixtures were written with tag 1, mark it reserved in a comment, or use tag 1 before this format ships.
• Stale comment in cast.rs: "retain the same immutable page index" refers to an index this PR no longer builds. The data().clone() simplification itself is correct, since ptype and slice bounds are unchanged and try_new re-validates.
• Hash/eq: ArrayEq/ArrayHash now include the full metadata, so a legacy array and an indexed array with identical compressed bytes no longer compare equal. That's probably fine, but it's worth one line in the PR body.
• Misleading test name: selection_does_not_validate_index_value_consistency runs take through the legacy full decoder, which never reads ends. So the test only shows that the field is ignored today. A name like inconsistent_index_is_ignored_by_legacy_decode would be more accurate.
• Style in lib.rs:
  • The doctest's use vortex_array::session::ArraySessionExt; moved below the let. Please move it back to the top.
  • use prost::Message; sits directly against mod array;. Add a blank line, or keep prost::Message qualified as before.
• pco_logical_pages_compat.rs: by default (PCO_COMPAT_MODE unset) this is just a write-and-read roundtrip. It never checks that the file actually contains logical_row_prefix. That's fine as scaffolding for the two-version check, but CI on its own proves nothing about compatibility.

Summary

The format change is additive and low-risk. Before merge:

1. Add the writer ends correctness test.
2. Restore the deleted chunk-size guard test.
3. Remove the avoidable Vec→Arc<[_]> copies, or explicitly accept the open-path regression.

I reviewed by reading the code only. I didn't build, run tests or run benchmarks.

[danking]

Addressed the avoidable open-path copy in 8d165ae: share Arc<Vec> rather than converting Vec into Arc<[ByteBuffer]>, preserving cheap view clones without copying the buffer descriptors into another allocation. Also fixed the doctest import placement and cast comment, and documented the experimental use of nested tag 1.

The oversized-chunk regression test is already restored in array.rs using Arc::make_mut. The requested wire-internal prefix assertions and semantic prefix/popcount scans were intentionally not added: Daniel explicitly requested public semantic tests and memory-safety-only validation. PR10825 now checks observable behavior across nullable chunk boundaries instead. The compressor's nonempty-page contract is unchanged. Metadata equality still distinguishes indexed and absent-index representations. Prior open/drop regressions remain disclosed; these changes were not rebenchmarked. Local independent source review passed; no builds/tests were run.