Skip to content

Commit 561d4ef

Browse files
dfa1claude
andcommitted
fix: render a null struct row as an empty CSV cell, not {"field":null,...}
A nested Struct-dtype column whose entire row is null rendered as a JSON object with every field null (e.g. {"id":null,"label":null}) instead of an empty CSV cell, diverging from the parquet oracle and from every other nullable column type in CsvExporter (discovered on the Raincloud oasst1 `labels` column while validating #268). Root cause: struct-row nullability is represented in this codebase by masking EACH field invalid (StructEncodingDecoder / StructLayoutDecoder wrap every field array in the shared struct validity), never by wrapping the whole StructArray in one MaskedArray. So a null struct row decodes as a StructArray whose fields are each MaskedArray-invalid; the StructArray itself is never masked. CsvExporter.cellValue routed a StructArray straight to jsonObject, which called jsonValue per field — each field's own MaskedArray arm rendered "null" — producing {"field":null,...} instead of an empty cell. Fix at the exporter, not the decoder. The per-field masking is deliberate and load-bearing: ScanIterator.expandStruct relies on the root StructArray being un-wrapped (its `instanceof StructArray` checks) and on each field carrying the struct-row validity so an expanded root column is null when the struct row is null. Wrapping the whole StructArray once would break that core scan path for every root struct and remove per-column validity propagation — a wide, risky change. The actual invariant is intact; the bug is purely that the exporter never recognized "every field invalid" as "the row is null". A new isNullStructRow(StructArray, long) helper detects that, and cellValue's and jsonValue's StructArray arms now render an empty cell / JSON null respectively. Added CsvExporterTest#rendersNullStructRowAsEmptyCell: a nested struct column with an all-fields-null row (the exact issue shape) asserts the cell is empty, not {"id":null,"label":null}. Verified it fails on the pre-fix exporter with the reported bad output. Review round (#275): added CsvExporterTest#rendersNullStructElementInListAsJsonNull to cover the nested jsonValue StructArray arm — a null struct element inside a List[Struct] must render as JSON null, not {"ref":null,...} — which was previously untested. Also clarified isNullStructRow's javadoc: it detects "every field invalid at this row", which is exact for struct-level-validity sources and an accepted approximation (indistinguishable from "present with all-null fields") for per-field-validity sources, matching the parquet oracle for the oasst1 case and unambiguous in practice since the writer cannot emit struct-level validity. Fixes #270 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 218c72f commit 561d4ef

2 files changed

Lines changed: 129 additions & 9 deletions

File tree

csv/src/main/java/io/github/dfa1/vortex/csv/CsvExporter.java

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -148,12 +148,13 @@ private static void writeChunk(Chunk chunk, List<String> colNames, int colCount,
148148
/// `Double.toString`, `Boolean.toString`, the raw utf8 string); a null row (a [MaskedArray]
149149
/// invalid row or a [NullArray]) renders as an empty CSV field.
150150
///
151-
/// A nested struct column ([StructArray], possibly wrapped in a [MaskedArray] when nullable)
152-
/// renders as a single JSON object cell `{"field":value,...}` with fields in the struct dtype's
153-
/// declared order. Field values reuse the same leaf rendering rules — numbers and booleans
154-
/// unquoted, strings JSON-escaped and double-quoted, nested structs recursed — and a null field
155-
/// (or null nested row) becomes a JSON `null`. Only the JSON layer is escaped here; the CSV
156-
/// writer independently quotes any cell containing the delimiter or a quote character.
151+
/// A nested struct column ([StructArray]) renders as a single JSON object cell
152+
/// `{"field":value,...}` with fields in the struct dtype's declared order, except a null struct
153+
/// row — decoded as every field masked invalid — which exports as an empty field like any other
154+
/// null. Field values reuse the same leaf rendering rules — numbers and booleans unquoted,
155+
/// strings JSON-escaped and double-quoted, nested structs recursed — and a null field (or null
156+
/// nested struct row) becomes a JSON `null`. Only the JSON layer is escaped here; the CSV writer
157+
/// independently quotes any cell containing the delimiter or a quote character.
157158
///
158159
/// A fixed-size list column ([FixedSizeListArray]) or variable-length list column ([ListArray]),
159160
/// either possibly wrapped in a [MaskedArray] when nullable, renders as a JSON array cell
@@ -192,8 +193,10 @@ private static String cellValue(Array arr, long rowIdx) {
192193
// All-null columns (DType.Null) hold only a row count: every cell is an empty
193194
// field, same rule as a MaskedArray null row.
194195
case NullArray ignored -> "";
195-
// Nested struct column: render the whole row as a JSON object cell.
196-
case StructArray sa -> jsonObject(sa, rowIdx);
196+
// Nested struct column: render the whole row as a JSON object cell, unless the row is
197+
// itself null (a null struct row is decoded as every field masked invalid, never as the
198+
// StructArray being wrapped once), which exports as an empty field like any other null.
199+
case StructArray sa -> isNullStructRow(sa, rowIdx) ? "" : jsonObject(sa, rowIdx);
197200
default -> throw new VortexException(
198201
"unsupported array type for CSV export: " + arr.getClass().getSimpleName());
199202
};
@@ -218,14 +221,40 @@ private static String jsonObject(StructArray struct, long rowIdx) {
218221
return sb.toString();
219222
}
220223

224+
/// Whether the struct row `rowIdx` should be treated as a null struct row. This detects "every
225+
/// field is a [MaskedArray] invalid at this row" and treats that as a null struct; a struct with
226+
/// no fields, or any field that is not maskable/valid, is never a null row.
227+
///
228+
/// The check is **exact** for struct-level-validity sources: `StructEncodingDecoder` represents a
229+
/// null struct row by wrapping every field (including non-nullable ones) in a [MaskedArray] that
230+
/// shares one struct-level validity bitmap, so all-fields-invalid means exactly "the row is null".
231+
/// For sources that only carry per-field validity it is an accepted **approximation** — a row
232+
/// where every field independently happens to be null is indistinguishable, on the wire, from a
233+
/// genuinely null row, and we pick the empty-cell interpretation. The distinction is moot in
234+
/// practice: `StructEncodingEncoder` cannot currently emit struct-level validity, so every null
235+
/// struct row this codebase produces already has the all-fields-invalid shape. This choice matches
236+
/// the parquet oracle's empty-cell rendering for the known conformance case (Raincloud `oasst1`).
237+
private static boolean isNullStructRow(StructArray struct, long rowIdx) {
238+
int fieldCount = struct.fieldCount();
239+
if (fieldCount == 0) {
240+
return false;
241+
}
242+
for (int i = 0; i < fieldCount; i++) {
243+
if (!(struct.field(i) instanceof MaskedArray ma) || ma.isValid(rowIdx)) {
244+
return false;
245+
}
246+
}
247+
return true;
248+
}
249+
221250
/// Renders one struct field value as JSON text: a nested object for structs, a quoted escaped
222251
/// string for utf8, JSON `null` for a null row/field, and the bare leaf rendering (unquoted
223252
/// number/boolean) for everything else. Unlike [#cellValue(Array, long)], a null here is the
224253
/// JSON token `null` rather than an empty field, because inside a JSON object the empty-field
225254
/// convention of bare CSV would be ambiguous.
226255
private static String jsonValue(Array arr, long rowIdx) {
227256
return switch (arr) {
228-
case StructArray sa -> jsonObject(sa, rowIdx);
257+
case StructArray sa -> isNullStructRow(sa, rowIdx) ? "null" : jsonObject(sa, rowIdx);
229258
case MaskedArray ma -> ma.isValid(rowIdx) ? jsonValue(ma.inner(), rowIdx) : "null";
230259
case FixedSizeListArray fla -> jsonArray(fla.elements(), rowIdx * fla.fixedSize(), (rowIdx + 1L) * fla.fixedSize());
231260
case ListArray la -> {

csv/src/test/java/io/github/dfa1/vortex/csv/CsvExporterTest.java

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.github.dfa1.vortex.writer.encode.BoolEncodingEncoder;
99
import io.github.dfa1.vortex.writer.encode.FixedSizeListData;
1010
import io.github.dfa1.vortex.writer.encode.ListData;
11+
import io.github.dfa1.vortex.writer.encode.ListEncodingEncoder;
1112
import io.github.dfa1.vortex.writer.encode.MaskedEncodingEncoder;
1213
import io.github.dfa1.vortex.writer.encode.NullEncodingEncoder;
1314
import io.github.dfa1.vortex.writer.encode.NullableData;
@@ -348,6 +349,49 @@ void rendersNullStructFieldAsJsonNull(@TempDir Path tmp) throws Exception {
348349
"south,\"{\"\"id\"\":2,\"\"label\"\":42}\"");
349350
}
350351

352+
@Test
353+
void rendersNullStructRowAsEmptyCell(@TempDir Path tmp) throws Exception {
354+
// Given a nested struct column whose whole row is null (issue #270): the struct decoder
355+
// represents a null struct row by masking every field invalid, never by wrapping the whole
356+
// StructArray once. The first row has both fields null (a null struct row); the second has
357+
// both valid. Regression: cellValue used to route straight to jsonObject, rendering the null
358+
// row as {"id":null,"label":null} instead of an empty cell like every other nullable column.
359+
// Two struct fields keep `data` from being unwrapped to its bare field on decode; the
360+
// `region` sibling keeps it off the single-column expand path.
361+
Path vortex = tmp.resolve("nullrow.vortex");
362+
DType.Struct inner = new DType.Struct(
363+
List.of(ColumnName.of("id"), ColumnName.of("label")),
364+
List.of(new DType.Primitive(PType.I64, true), new DType.Primitive(PType.I64, true)),
365+
false);
366+
DType.Struct schema = new DType.Struct(
367+
List.of(ColumnName.of("region"), ColumnName.of("data")),
368+
List.of(DType.UTF8, inner),
369+
false);
370+
try (FileChannel ch = FileChannel.open(vortex, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
371+
VortexWriter writer = VortexWriter.create(ch, schema, WriteOptions.defaults(),
372+
List.of(new StructEncodingEncoder(), new PrimitiveEncodingEncoder(),
373+
new VarBinEncodingEncoder(), new MaskedEncodingEncoder(),
374+
new BoolEncodingEncoder()))) {
375+
writer.writeChunk(Map.of(
376+
ColumnName.of("region"), new String[]{"north", "south"},
377+
ColumnName.of("data"), new StructData(List.of(
378+
new NullableData(new long[]{0L, 1L}, new boolean[]{false, true}),
379+
new NullableData(new long[]{0L, 42L}, new boolean[]{false, true})))));
380+
}
381+
Path csv = tmp.resolve("out.csv");
382+
383+
// When
384+
CsvExporter.exportCsv(vortex, csv);
385+
386+
// Then the all-fields-null row is an empty cell (not {"id":null,"label":null}); the valid
387+
// row is the JSON object.
388+
List<String> result = Files.readAllLines(csv);
389+
assertThat(result).containsExactly(
390+
"region,data",
391+
"north,",
392+
"south,\"{\"\"id\"\":1,\"\"label\"\":42}\"");
393+
}
394+
351395
@Test
352396
void escapesQuotesAndCommasInsideStructStringField(@TempDir Path tmp) throws Exception {
353397
// Given a struct string field value containing a JSON-significant quote and a comma: the
@@ -521,4 +565,51 @@ void rendersListOfStructsAsJsonArrayOfObjects(@TempDir Path tmp) throws Exceptio
521565
"id,members",
522566
"1,\"[{\"\"ref\"\":10,\"\"role\"\":\"\"\"\"},{\"\"ref\"\":20,\"\"role\"\":\"\"outer\"\"}]\"");
523567
}
568+
569+
@Test
570+
void rendersNullStructElementInListAsJsonNull(@TempDir Path tmp) throws Exception {
571+
// Given a List[Struct] whose first element is a null struct row (issue #270, nested case):
572+
// the struct decoder masks every field invalid, so cellValue never reaches this element —
573+
// jsonValue's StructArray arm does, and must emit the JSON token null (not
574+
// {"ref":null,"role":null}) inside the array. This is the only test exercising jsonValue's
575+
// isNullStructRow branch; the sibling rendersNullStructRowAsEmptyCell only covers the
576+
// top-level cellValue path. Each field is an independent NullableData both invalid at row 0
577+
// (the all-fields-invalid shape a genuinely-null struct row decodes to), matching the
578+
// construction rendersNullStructRowAsEmptyCell uses. The struct needs two fields so it is
579+
// not unwrapped on decode; the `id` sibling keeps the list column off the single-column path.
580+
Path vortex = tmp.resolve("nullstructinlist.vortex");
581+
DType.Struct memberDtype = new DType.Struct(
582+
List.of(ColumnName.of("ref"), ColumnName.of("role")),
583+
List.of(new DType.Primitive(PType.I64, true), new DType.Primitive(PType.I64, true)),
584+
false);
585+
DType.List listDtype = new DType.List(memberDtype, false);
586+
DType.Struct schema = new DType.Struct(
587+
List.of(ColumnName.of("id"), ColumnName.of("members")),
588+
List.of(DType.I64, listDtype),
589+
false);
590+
try (FileChannel ch = FileChannel.open(vortex, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
591+
VortexWriter writer = VortexWriter.create(ch, schema, WriteOptions.defaults(),
592+
List.of(new ListEncodingEncoder(), new StructEncodingEncoder(),
593+
new PrimitiveEncodingEncoder(), new VarBinEncodingEncoder(),
594+
new MaskedEncodingEncoder(), new BoolEncodingEncoder()))) {
595+
writer.writeChunk(Map.of(
596+
ColumnName.of("id"), new long[]{1L},
597+
ColumnName.of("members"), new ListData(
598+
new StructData(List.of(
599+
new NullableData(new long[]{0L, 20L}, new boolean[]{false, true}),
600+
new NullableData(new long[]{0L, 42L}, new boolean[]{false, true}))),
601+
new long[]{0, 2},
602+
1)));
603+
}
604+
Path csv = tmp.resolve("out.csv");
605+
606+
// When
607+
CsvExporter.exportCsv(vortex, csv);
608+
List<String> result = Files.readAllLines(csv);
609+
610+
// Then the null struct element is the JSON token null; the valid one is a JSON object.
611+
assertThat(result).containsExactly(
612+
"id,members",
613+
"1,\"[null,{\"\"ref\"\":20,\"\"role\"\":42}]\"");
614+
}
524615
}

0 commit comments

Comments
 (0)