fix: scan misaligned per-column chunk grids (#221)#223
Merged
Conversation
dfa1
added a commit
that referenced
this pull request
Jul 7, 2026
Addresses PR #223 review findings. 1. Ground-truth value test for the coarse-sub-chunk slice path. Neither writer can EMIT a disjoint per-column chunk grid — the Java VortexWriter's writeChunk requires all columns to agree on row count, and the JNI writer writes uniform row batches — so a genuine mixed grid ([3,3,2] vs [4,4]) is hand-assembled at the file-format level (reusing VortexReaderDecodeChunkTest's raw-file seam) and the full streaming scan is asserted value-for-value, including "a"'s middle chunk sliced at a nonzero offset and a nullable column crossing the MaskedArray slice. Rust-parity stays covered by RaincloudConformanceIntegrationTest. 2. Evict cached coarse flats once passed. Each coarse covering flat now decodes into its own confined arena; as the scan advances, a column whose covering flat changes from the previous decoded window releases the earlier flat's arena (the planner's per-column cursor is monotonic, so it is never revisited). The previous Chunk is already closed at that point, so no live slice references the freed arena. Bounds peak off-heap memory for wide disjoint-grid files instead of retaining every decoded chunk for the iterator's lifetime. The new streaming test iterates fully, proving later windows still decode correctly after earlier chunks are evicted. 3. Reword the columnZoneStats fallback javadoc: entries are now per scan window, with a coarse chunk's stats repeated (conservatively) across its sub-windows. 4. ScanIteratorChunkGridTest states column order via a LinkedHashMap helper rather than relying on Map.of iteration order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ScanIterator.buildChunks rejected any per-column chunk mismatch beyond the
narrow 1-vs-N case ("mixed per-column chunking beyond 1-vs-N is not supported"),
so two Raincloud files the Rust oracle reads fine failed to scan.
Replace the 1-vs-N special case with the Rust reference's policy: the scan
planner now splits at the merged boundary grid — the sorted, deduplicated union
of every column's chunk boundaries. This mirrors vortex-layout's
StructReader::register_splits, which unions each field child's chunk boundaries
into one RowSplits set (vortex-layout/src/scan/split_by.rs), the scan then
walking each adjacent-boundary window and slicing every column's covering chunk
to it. The merged grid refines every column's grid, so each window lies entirely
within exactly one chunk of each column.
Tier decision: the merged-grid generalization is tractable and subsumes both
tiers from the issue, so it is implemented in full rather than only the nested
tier:
- nested boundaries (emotions-dataset-for-nlp: label [131072 x3, 23593] nests
inside text's 16384 grid), and
- disjoint boundaries (uci-beijing-multi-site-air-quality: numeric
[131072 x3, 27552] vs station's 40960/49152 grid, which do not nest)
both fall out of the union with no tier-specific code.
Fast paths are preserved: when a window equals a whole covering chunk (offset 0,
window rows == flat rows) the chunk is decoded straight into the chunk's arena
with no slice wrapper, so aligned N-vs-N and per-chunk decode stay zero-copy. A
coarse chunk that spans several windows is decoded once into the iterator's
shared arena (cached by identity) and sliced zero-copy per window, never
re-decoded.
Corpus evidence (CLI export vs the Parquet oracle):
uci-beijing-multi-site-air-quality: OK (420768 rows x 18 cols)
emotions-dataset-for-nlp: OK (416809 rows x 2 cols)
Both matrix entries flipped gap:221 -> ok; conformance suite green.
Closes #221
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses PR #223 review findings. 1. Ground-truth value test for the coarse-sub-chunk slice path. Neither writer can EMIT a disjoint per-column chunk grid — the Java VortexWriter's writeChunk requires all columns to agree on row count, and the JNI writer writes uniform row batches — so a genuine mixed grid ([3,3,2] vs [4,4]) is hand-assembled at the file-format level (reusing VortexReaderDecodeChunkTest's raw-file seam) and the full streaming scan is asserted value-for-value, including "a"'s middle chunk sliced at a nonzero offset and a nullable column crossing the MaskedArray slice. Rust-parity stays covered by RaincloudConformanceIntegrationTest. 2. Evict cached coarse flats once passed. Each coarse covering flat now decodes into its own confined arena; as the scan advances, a column whose covering flat changes from the previous decoded window releases the earlier flat's arena (the planner's per-column cursor is monotonic, so it is never revisited). The previous Chunk is already closed at that point, so no live slice references the freed arena. Bounds peak off-heap memory for wide disjoint-grid files instead of retaining every decoded chunk for the iterator's lifetime. The new streaming test iterates fully, proving later windows still decode correctly after earlier chunks are evicted. 3. Reword the columnZoneStats fallback javadoc: entries are now per scan window, with a coarse chunk's stats repeated (conservatively) across its sub-windows. 4. ScanIteratorChunkGridTest states column order via a LinkedHashMap helper rather than relying on Map.of iteration order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Review findings applied in 0840419:
Rebased onto main (past #197's zoned-stats change — they touch the same zone path; auto-merged textually, verified semantically: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #221, found by Raincloud triage round 3 (#205):
ScanIterator.buildChunksrejected files whose columns use different chunk grids ("mixed per-column chunking beyond 1-vs-N is not supported") — files the Rust reader handles fine.Approach mirrors the Rust reference (
vortex-layout/src/scan/split_by.rs+register_splitsdown the layout tree): scan windows are the sorted-deduped union of every column's chunk boundaries. For each window, each column advances a monotonic cursor to its covering chunk and slices to the window. This one model subsumes both failure tiers — nested boundaries (emotions) and truly disjoint grids (beijing) — with no tier-specific code.No regression to the fast paths:
isWholeFlatgates a window that equals a whole chunk straight into that chunk's arena (aligned N-vs-N and 1-vs-N keep per-chunk zero-copy, no slice wrapper). A coarse chunk spanning several windows is decoded once into a shared arena and sliced zero-copy per window.Validation:
ScanIteratorChunkGridTest(aligned, 1-vs-N, nested[8,8,8,4]-vs-[2×14], disjoint[3,3,2]-vs-[4,4], empty).verify -pl integration -am287 ITs green (no scan regressions).uci-beijing-multi-site-air-quality(7.5M cells) andemotions-dataset-for-nlp(833k cells) both export and match the Parquet oracle exactly.gap:221→ok(30 ok / 0 gaps); docs + CHANGELOG updated.Reviewer note (from the agent): coarse covering flats are cached for the iterator's lifetime in a shared arena — correctness and decode-once are satisfied; per-chunk arena eviction on a very wide disjoint file is a possible future memory optimization, not a correctness issue.
Closes #221
🤖 Generated with Claude Code