Skip to content

Commit 02ce276

Browse files
dfa1claude
andcommitted
review: ground-truth mixed-grid test + evict passed coarse flats (#221)
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>
1 parent 154c7e6 commit 02ce276

3 files changed

Lines changed: 197 additions & 31 deletions

File tree

reader/src/main/java/io/github/dfa1/vortex/reader/ScanIterator.java

Lines changed: 59 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,20 @@ public final class ScanIterator implements Iterator<Chunk>, AutoCloseable {
8787
private long rowsReturned;
8888
private Chunk openChunk;
8989
private boolean closed;
90-
private Arena sharedArena;
91-
private Map<Layout, Array> sharedFlats;
90+
// Coarse chunks that span several split windows are decoded once and cached here (keyed by
91+
// layout identity), each in its own confined arena so it can be released the moment the scan
92+
// advances past its last covering window — see evictPassedFlats.
93+
private Map<Layout, CachedFlat> sharedFlats;
94+
// Per projected column, the covering flat of the most recently decoded window; drives eviction.
95+
private Layout[] lastCoveringFlats;
96+
97+
/// A coarse covering flat decoded once and sliced across the windows it spans, together with
98+
/// the confined [Arena] that owns its buffers (closed on eviction / iterator close).
99+
///
100+
/// @param arena the confined arena owning this flat's decoded buffers
101+
/// @param array the decoded flat, sliced per window
102+
private record CachedFlat(Arena arena, Array array) {
103+
}
92104

93105
public ScanIterator(VortexHandle file, ScanOptions options) {
94106
this.file = file;
@@ -295,6 +307,8 @@ public Chunk next() {
295307
chunkIndex = peekedChunkIdx + 1;
296308
peekedChunkIdx = -1;
297309

310+
evictPassedFlats(spec);
311+
298312
long remaining = options.limit() - rowsReturned;
299313
long chunkRows = Math.min(spec.rowCount(), remaining);
300314

@@ -390,11 +404,14 @@ public long[] chunkRowCounts() {
390404
/// `null`, since the flat writer does not retain it). Either way a column that is absent
391405
/// or carries no stats yields [ArrayStats#empty()] per zone.
392406
///
393-
/// Zone granularity is the layout's, not the scan's. The fallback path is one entry per
394-
/// chunk, positionally aligned with [#chunkRowCounts()]. The zone-map path is one entry per
395-
/// zone of the stats table: this writer emits one zone per chunk (so the same alignment
396-
/// holds), but a file from another writer may use a fixed zone length independent of chunk
397-
/// boundaries, in which case the zone count need not match [#chunkRowCounts()].
407+
/// Zone granularity is the layout's, not the scan's. The fallback path is one entry per scan
408+
/// window, positionally aligned with [#chunkRowCounts()]. Each entry carries the stats of the
409+
/// covering chunk of that window, so when columns chunk on different grids a coarse chunk's
410+
/// stats repeat across every sub-window it spans (still a conservative min/max/null-count bound
411+
/// for each sub-window). The zone-map path is one entry per zone of the stats table: this writer
412+
/// emits one zone per chunk (so the same alignment holds), but a file from another writer may
413+
/// use a fixed zone length independent of chunk boundaries, in which case the zone count need
414+
/// not match [#chunkRowCounts()].
398415
///
399416
/// This is the read-side surface for aggregate push-down (ADR 0013 §6): a reduction can
400417
/// fold whole zones from these rows and fall back to a streaming decode only for the
@@ -564,9 +581,10 @@ public void close() {
564581
openChunk.close();
565582
}
566583
openChunk = null;
567-
if (sharedArena != null) {
568-
sharedArena.close();
569-
sharedArena = null;
584+
if (sharedFlats != null) {
585+
for (CachedFlat cached : sharedFlats.values()) {
586+
cached.arena().close();
587+
}
570588
sharedFlats = null;
571589
}
572590
}
@@ -608,6 +626,7 @@ private void initialize() {
608626

609627
projectedNames = List.copyOf(columnDtypes.keySet());
610628
projectedDtypes = List.copyOf(columnDtypes.values());
629+
lastCoveringFlats = new Layout[projectedNames.size()];
611630
chunks = buildChunks(columnFlats);
612631
}
613632

@@ -682,8 +701,8 @@ private Array decodeOrSlice(int colIdx, Layout coveringFlat, long sliceOffset, l
682701
// no slice wrapper (aligned N-vs-N and per-chunk decode keep their zero-copy behavior).
683702
return decodeLayout(coveringFlat, dtype, arena);
684703
}
685-
// Covering chunk spans several windows: decode it once into the shared arena and slice
686-
// zero-copy per window, matching how Rust decodes a chunk once and slices each split range.
704+
// Covering chunk spans several windows: decode it once (cached) and slice zero-copy per
705+
// window, matching how Rust decodes a chunk once and slices each split range.
687706
Array full = sharedCoveringFlat(coveringFlat, dtype);
688707
return sliceArray(full, sliceOffset, rowCount, dtype);
689708
}
@@ -692,14 +711,37 @@ private static boolean isWholeFlat(Layout coveringFlat, long sliceOffset, long r
692711
return sliceOffset == 0 && rowCount == coveringFlat.rowCount();
693712
}
694713

695-
/// Decodes `flat` once into the iterator's shared arena and caches it by identity, so a coarse
696-
/// chunk that covers several split windows is decoded a single time and then sliced per window.
714+
/// Decodes `flat` once into its own confined arena and caches it by identity, so a coarse chunk
715+
/// that covers several split windows is decoded a single time and then sliced per window. The
716+
/// arena is released by [#evictPassedFlats] once the scan advances off the flat.
697717
private Array sharedCoveringFlat(Layout flat, DType dtype) {
698-
if (sharedArena == null) {
699-
sharedArena = Arena.ofConfined();
718+
if (sharedFlats == null) {
700719
sharedFlats = new IdentityHashMap<>();
701720
}
702-
return sharedFlats.computeIfAbsent(flat, f -> decodeLayout(f, dtype, sharedArena));
721+
return sharedFlats.computeIfAbsent(flat, f -> {
722+
Arena flatArena = Arena.ofConfined();
723+
return new CachedFlat(flatArena, decodeLayout(f, dtype, flatArena));
724+
}).array();
725+
}
726+
727+
/// Releases every cached coarse flat the scan has moved past. As `spec` is decoded, a column
728+
/// whose covering flat differs from the one it used in the previously decoded window can never
729+
/// revisit that earlier flat — the planner's per-column cursor advances monotonically — so its
730+
/// arena is closed and its cache entry dropped. Whole-window (uncached) flats are ignored. The
731+
/// previously open [Chunk] is already closed here (enforced by [#next()]), so no live slice
732+
/// still references the freed arena.
733+
private void evictPassedFlats(ChunkSpec spec) {
734+
Layout[] current = spec.columnLayouts();
735+
for (int j = 0; j < current.length; j++) {
736+
Layout previous = lastCoveringFlats[j];
737+
if (previous != null && previous != current[j]) {
738+
CachedFlat cached = sharedFlats == null ? null : sharedFlats.remove(previous);
739+
if (cached != null) {
740+
cached.arena().close();
741+
}
742+
}
743+
lastCoveringFlats[j] = current[j];
744+
}
703745
}
704746

705747
private static Array sliceArray(Array full, long offset, long length, DType dtype) {

reader/src/test/java/io/github/dfa1/vortex/reader/ScanIteratorChunkGridTest.java

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ class ScanIteratorChunkGridTest {
2626
@Test
2727
void alignedGridsDecodeEveryColumnAsAWholeChunk() {
2828
// Given two columns with the *same* grid [4, 4] over 8 rows — the aligned N-vs-N fast path.
29-
var columnFlats = flats(Map.of(A, new long[]{4, 4}, B, new long[]{4, 4}));
29+
var columnFlats = flats(A, new long[]{4, 4}, B, new long[]{4, 4});
3030

3131
// When
3232
List<ChunkSpec> result = ScanIterator.buildChunks(columnFlats);
@@ -43,7 +43,7 @@ void alignedGridsDecodeEveryColumnAsAWholeChunk() {
4343
@Test
4444
void singleFlatColumnSharesTheChunkedGrid() {
4545
// Given one full-column flat [8] beside a chunked column [4, 4] — the 1-vs-N case.
46-
var columnFlats = flats(Map.of(A, new long[]{8}, B, new long[]{4, 4}));
46+
var columnFlats = flats(A, new long[]{8}, B, new long[]{4, 4});
4747

4848
// When
4949
List<ChunkSpec> result = ScanIterator.buildChunks(columnFlats);
@@ -62,9 +62,9 @@ void nestedBoundariesSliceTheCoarseColumnAtTheFineGrid() {
6262
// Given the emotions-dataset-for-nlp shape scaled down: a coarse column [8, 8, 8, 4] (like
6363
// `label`'s [131072 ×3, 23593]) beside a fine column of 2-row chunks (like `text`'s 16384
6464
// grid, where 8 = 4 × 2 so every coarse boundary is also a fine boundary).
65-
var columnFlats = flats(Map.of(
65+
var columnFlats = flats(
6666
A, new long[]{8, 8, 8, 4},
67-
B, new long[]{2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2}));
67+
B, new long[]{2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2});
6868

6969
// When
7070
List<ChunkSpec> result = ScanIterator.buildChunks(columnFlats);
@@ -94,9 +94,9 @@ void disjointBoundariesSpliceAtTheMergedGrid() {
9494
// Given the uci-beijing-multi-site-air-quality shape scaled down: [3, 3, 2] vs [4, 4] over 8
9595
// rows. The boundaries do not nest (3 + 3 = 6 != 4, mirroring 40960 + 49152 != 131072), so
9696
// neither grid refines the other — the scan must emit windows at the union of boundaries.
97-
var columnFlats = flats(Map.of(
97+
var columnFlats = flats(
9898
A, new long[]{3, 3, 2},
99-
B, new long[]{4, 4}));
99+
B, new long[]{4, 4});
100100

101101
// When
102102
List<ChunkSpec> result = ScanIterator.buildChunks(columnFlats);
@@ -134,18 +134,24 @@ void emptyProjectionYieldsNoChunks() {
134134

135135
// ── helpers ───────────────────────────────────────────────────────────────
136136

137-
private static Map<ColumnName, List<Layout>> flats(Map<ColumnName, long[]> grids) {
137+
/// Builds an insertion-ordered `column -> flats` map for two columns. A [LinkedHashMap] states
138+
/// the intended column order explicitly (the planner's output is order-independent and read
139+
/// back by name here, but an unordered [Map#of] would hide that intent).
140+
private static Map<ColumnName, List<Layout>> flats(ColumnName n1, long[] g1, ColumnName n2, long[] g2) {
138141
var out = new LinkedHashMap<ColumnName, List<Layout>>();
139-
for (var entry : grids.entrySet()) {
140-
var list = new ArrayList<Layout>();
141-
for (long rows : entry.getValue()) {
142-
list.add(new Layout(LayoutId.FLAT, rows, null, List.of(), List.of()));
143-
}
144-
out.put(entry.getKey(), list);
145-
}
142+
out.put(n1, toFlats(g1));
143+
out.put(n2, toFlats(g2));
146144
return out;
147145
}
148146

147+
private static List<Layout> toFlats(long[] chunkRows) {
148+
var list = new ArrayList<Layout>(chunkRows.length);
149+
for (long rows : chunkRows) {
150+
list.add(new Layout(LayoutId.FLAT, rows, null, List.of(), List.of()));
151+
}
152+
return list;
153+
}
154+
149155
private static List<Long> rowCounts(List<ChunkSpec> chunks) {
150156
var out = new ArrayList<Long>(chunks.size());
151157
for (ChunkSpec spec : chunks) {

reader/src/test/java/io/github/dfa1/vortex/reader/VortexReaderDecodeChunkTest.java

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,54 @@ void decodeChunk_sharedSingleFlatColumn_isValueEquivalentAndSurvivesIteratorClos
262262
}
263263
}
264264

265+
// A genuinely DISJOINT per-column chunk grid (issue #221, "beijing" tier): column "a" chunks
266+
// [3, 3, 2] and column "b" chunks [4, 4] over the same 8 rows. The boundaries do not nest
267+
// ({0,3,6,8} vs {0,4,8}), so neither grid refines the other; the scan must split at the union
268+
// {0,3,4,6,8}. "b" is nullable so the coarse-chunk slice also crosses the MaskedArray branch.
269+
private static final long[] MIXED_A_ROWS = {3, 3, 2};
270+
private static final long[][] MIXED_A = {{10, 11, 12}, {13, 14, 15}, {16, 17}};
271+
private static final long[] MIXED_B_ROWS = {4, 4};
272+
private static final long[][] MIXED_B = {{20, 21, 22, 23}, {24, 25, 26, 27}};
273+
private static final boolean[][] MIXED_B_VALID = {{true, false, true, true}, {true, true, false, true}};
274+
private static final List<Long> MIXED_A_EXPECTED = List.of(10L, 11L, 12L, 13L, 14L, 15L, 16L, 17L);
275+
private static final List<Long> MIXED_B_EXPECTED =
276+
java.util.Arrays.asList(20L, null, 22L, 23L, 24L, 25L, null, 27L);
277+
278+
@Test
279+
void scan_disjointMixedGrid_streamsExactValuesAcrossMergedWindows(@TempDir Path tmp) throws Exception {
280+
// This is the value-level ground truth for #221's coarse-sub-chunk slice path. Neither the
281+
// Java VortexWriter (its writeChunk requires all columns to agree on row count) nor the JNI
282+
// writer (it writes uniform row batches) can EMIT a disjoint per-column grid, so the file is
283+
// hand-assembled here; Rust-parity of the fix is covered separately by the size-gated
284+
// RaincloudConformanceIntegrationTest (uci-beijing-multi-site-air-quality). Streaming the
285+
// whole scan exercises the merged-grid planner end to end AND, by fully iterating, drives
286+
// the coarse-flat eviction path (earlier chunks are released while later windows must still
287+
// decode correctly). The ground truth is the exact values written into each segment.
288+
289+
// Given a file whose two columns chunk on disjoint grids [3,3,2] vs [4,4]
290+
Path file = writeMixedGridFile(tmp);
291+
var windowRowCounts = new ArrayList<Long>();
292+
var streamedA = new ArrayList<Long>();
293+
var streamedB = new ArrayList<Long>();
294+
295+
// When streaming the whole scan
296+
try (var reader = VortexReader.open(file, registry());
297+
var iter = reader.scan(ScanOptions.all())) {
298+
iter.forEachRemaining(chunk -> {
299+
windowRowCounts.add(chunk.rowCount());
300+
streamedA.addAll(values(chunk.column("a")));
301+
streamedB.addAll(values(chunk.column("b")));
302+
});
303+
}
304+
305+
// Then the scan splits at the merged grid {0,3,4,6,8} -> windows of 3,1,2,2 rows, and every
306+
// value (and null) survives the coarse-chunk sub-window slice — including "a"'s middle chunk
307+
// sliced at a nonzero offset for window [4,6) and "b"'s first chunk sliced for [0,3)+[3,4).
308+
assertThat(windowRowCounts).containsExactly(3L, 1L, 2L, 2L);
309+
assertThat(streamedA).isEqualTo(MIXED_A_EXPECTED);
310+
assertThat(streamedB).isEqualTo(MIXED_B_EXPECTED);
311+
}
312+
265313
// ── Helpers ────────────────────────────────────────────────────────────────
266314

267315
private static ReadRegistry registry() {
@@ -302,6 +350,76 @@ private static Path writeMultiChunkFile(Path dir) throws Exception {
302350
return writeFile(dir, "multi.vtx", CHUNK_ROWS, COL_A, COL_B, COL_B_VALID);
303351
}
304352

353+
/// Assembles a struct file whose columns chunk on independent grids: "a" over [MIXED_A_ROWS]
354+
/// (non-nullable) and "b" over [MIXED_B_ROWS] (nullable). Segment order: a-chunk0..a-chunkN,
355+
/// then b-chunk0..b-chunkM. Layout: flat=0, chunked=1, struct=2; array specs: primitive=0,
356+
/// bool=1.
357+
private static Path writeMixedGridFile(Path dir) throws Exception {
358+
List<byte[]> segments = new ArrayList<>();
359+
for (long[] chunk : MIXED_A) {
360+
segments.add(primitiveSegment(chunk, null));
361+
}
362+
int bBase = segments.size();
363+
for (int k = 0; k < MIXED_B.length; k++) {
364+
segments.add(primitiveSegment(MIXED_B[k], MIXED_B_VALID[k]));
365+
}
366+
367+
long[] segOffsets = new long[segments.size()];
368+
long[] segLengths = new long[segments.size()];
369+
long off = 0;
370+
for (int i = 0; i < segments.size(); i++) {
371+
segOffsets[i] = off;
372+
segLengths[i] = segments.get(i).length;
373+
off += segments.get(i).length;
374+
}
375+
376+
int[] aSeg = new int[MIXED_A_ROWS.length];
377+
for (int k = 0; k < aSeg.length; k++) {
378+
aSeg[k] = k;
379+
}
380+
int[] bSeg = new int[MIXED_B_ROWS.length];
381+
for (int k = 0; k < bSeg.length; k++) {
382+
bSeg[k] = bBase + k;
383+
}
384+
385+
ByteBuffer footerBuf = MalformedFiles.buildFooter(
386+
new String[]{"vortex.primitive", "vortex.bool"},
387+
new String[]{"vortex.flat", "vortex.chunked", "vortex.struct"},
388+
segOffsets, segLengths);
389+
ByteBuffer dtypeBuf = buildStructDtype();
390+
ByteBuffer layoutBuf = buildMixedLayout(aSeg, MIXED_A_ROWS, bSeg, MIXED_B_ROWS);
391+
392+
return writeVtxFile(dir, "mixed.vtx", segments, footerBuf, dtypeBuf, layoutBuf);
393+
}
394+
395+
/// Layout for the disjoint-grid file: two chunked columns whose flats carry independent
396+
/// row counts, so the reader's planner must split at the union of both grids.
397+
private static ByteBuffer buildMixedLayout(int[] aSeg, long[] aRows, int[] bSeg, long[] bRows) {
398+
var fbb = new FbsBuilder(1024);
399+
long total = 0;
400+
for (long r : aRows) {
401+
total += r;
402+
}
403+
int chunkedA = buildChunkedColumn(fbb, aSeg, aRows, total);
404+
int chunkedB = buildChunkedColumn(fbb, bSeg, bRows, total);
405+
int structChildV = FbsLayout.createChildrenVector(fbb, new int[]{chunkedA, chunkedB});
406+
int structOff = FbsLayout.createFbsLayout(fbb, 2, total, 0, structChildV, 0);
407+
FbsLayout.finishFbsLayoutBuffer(fbb, structOff);
408+
return MalformedFiles.slice(fbb);
409+
}
410+
411+
/// Builds one `vortex.chunked` layout node whose children are one `vortex.flat` per chunk,
412+
/// each flat carrying its own `rows[k]` row count and single segment index `segIdx[k]`.
413+
private static int buildChunkedColumn(FbsBuilder fbb, int[] segIdx, long[] rows, long total) {
414+
int[] flatOffs = new int[rows.length];
415+
for (int k = 0; k < rows.length; k++) {
416+
int segV = FbsLayout.createSegmentsVector(fbb, new long[]{segIdx[k]});
417+
flatOffs[k] = FbsLayout.createFbsLayout(fbb, 0, rows[k], 0, 0, segV);
418+
}
419+
int childV = FbsLayout.createChildrenVector(fbb, flatOffs);
420+
return FbsLayout.createFbsLayout(fbb, 1, total, 0, childV, 0);
421+
}
422+
305423
/// Streams the whole scan of the shared-column file, collecting the per-chunk column "a" and
306424
/// the per-chunk slice of the single-flat shared column "c" so a random-access decode can be
307425
/// compared element-by-element.

0 commit comments

Comments
 (0)