Skip to content

Commit d3b5b25

Browse files
dfa1claude
andcommitted
fix(reader): reject duplicate field names and arity desync in file schema
Completes the column-identity work: PostscriptParser rejects a file schema whose struct declares duplicate field names (VortexException; the name-keyed Chunk API would silently drop a column) or mismatched names/dtypes lengths. Deliberate read-side divergence from the Rust reader's tolerance, documented in docs/compatibility.md: the reference WRITER refuses such files ("StructLayout must have unique field names"), so tolerating them buys nothing and silent column loss is worse than a loud failure. DType.Struct's canonical constructor now validates non-null lists and 1:1 arity; duplicate names stay legal in memory, mirroring Rust's StructFields — uniqueness is a file-boundary contract. CLAUDE.md file-format note corrected: the dtype blob is FlatBuffers (FbsDType), not Protobuf. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent dca815b commit d3b5b25

7 files changed

Lines changed: 177 additions & 19 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ git push && git push --tags # GitHub Actions deploys the tag to Maven C
115115

116116
8-byte trailer at EOF: `version(u16 LE) | postscriptLen(u16 LE) | magic(VTXF)`. The postscript
117117
(FlatBuffer, immediately before the trailer) points (offset+length) to the Footer (FlatBuffer),
118-
DType (Protobuf), and Layout (FlatBuffer) blobs elsewhere in the file.
118+
DType (FlatBuffer), and Layout (FlatBuffer) blobs elsewhere in the file.
119119

120120
Layout tree: `Struct → Zoned(Stats) → Chunked → [Flat, Flat, ...]`
121121
- **Flat** single encoded segment · **Chunked** sequence of Flats · **Struct** one child/column

TODO.md

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -95,23 +95,6 @@ Per-encoding gotchas:
9595
- See [ADR-0008](adr/0008-domain-primitives-unsigned-integers.md) and https://dfa1.github.io/articles/rethink-domain-primitives-with-valhalla
9696
- Candidates: `PType` integer kinds, buffer offsets, row indices, byte lengths
9797
- Goal: type-safety at zero cost (value class = no heap alloc, no boxing)
98-
- [ ] **Column identity: duplicate/empty field names — remaining reader-side work** — measured
99-
against the JNI oracle 2026-07-04 (`ColumnNameEdgeCasesIntegrationTest` pins the facts):
100-
- `""` is a legal column name: round-trips both directions, DONE (pinned by tests).
101-
- Duplicate names: Rust's in-memory `StructFields` allows them (first-match access) but its
102-
FILE writer rejects them ("StructLayout must have unique field names"). Our writer now
103-
mirrors that guard, DONE. The Rust READER tolerates a foreign duplicate-name file
104-
(schema reports both fields); ours silently collapses them in `Chunk`'s
105-
`Map<String, Array>` — a crafted file loses a column with no error. Open: reject loudly at
106-
the parse edge (cheap, diverges from Rust reader tolerance) vs ordered first-match `Chunk`
107-
columns (Rust-aligned, reshapes the `columns()` API) — ADR when picked up.
108-
- `DType.Struct` record constructor validates nothing (arity `fieldNames`/`fieldTypes` desync
109-
possible; only `StructBuilder` checks) — consider canonical-constructor validation.
110-
- Unexplained: a duplicate-name write produced two chunks from one `writeChunk` (pre-guard);
111-
understand before it bites elsewhere.
112-
- `ColumnName` domain primitive verdict: per-value invariant is thin (`""` is legal); the real
113-
invariant is structural and lives on the struct/file layer, exactly as the reference's error
114-
message says.
11598

11699
## Compute
117100

core/src/main/java/io/github/dfa1/vortex/core/model/DType.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,25 @@ record Struct(
245245
java.util.List<DType> fieldTypes,
246246
boolean nullable
247247
) implements DType {
248+
249+
/// Validates that names and types pair 1:1. Duplicate names are legal in memory,
250+
/// mirroring the Rust reference's `StructFields`; uniqueness is a file-boundary
251+
/// contract, enforced by the writer and the file parser.
252+
///
253+
/// @param fieldNames ordered list of field names
254+
/// @param fieldTypes ordered list of field types, parallel to `fieldNames`
255+
/// @param nullable whether null values are permitted
256+
/// @throws NullPointerException if `fieldNames` or `fieldTypes` is `null`
257+
/// @throws IllegalArgumentException if the two lists differ in size
258+
public Struct {
259+
java.util.Objects.requireNonNull(fieldNames, "fieldNames");
260+
java.util.Objects.requireNonNull(fieldTypes, "fieldTypes");
261+
if (fieldNames.size() != fieldTypes.size()) {
262+
throw new IllegalArgumentException("fieldNames/fieldTypes size mismatch: "
263+
+ fieldNames.size() + " names, " + fieldTypes.size() + " types");
264+
}
265+
}
266+
248267
/// Returns the type of the field with the given name.
249268
///
250269
/// @param name the field name to look up
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package io.github.dfa1.vortex.core.model;
2+
3+
import org.junit.jupiter.api.Test;
4+
5+
import java.util.List;
6+
7+
import static org.assertj.core.api.Assertions.assertThat;
8+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
9+
10+
/// Canonical-constructor validation of [DType.Struct]: names and types must pair 1:1.
11+
/// Duplicate names stay legal in memory — mirroring the Rust reference's `StructFields` —
12+
/// because uniqueness is a file-boundary contract (writer guard + parser guard), not a
13+
/// property of the in-memory type.
14+
class DTypeStructValidationTest {
15+
16+
private static final DType I64 = new DType.Primitive(PType.I64, false);
17+
18+
@Test
19+
void construct_arityMismatch_throwsIllegalArgumentException() {
20+
// Given / When / Then — two names, one type: the record constructor rejects the desync
21+
assertThatThrownBy(() -> new DType.Struct(List.of("a", "b"), List.of(I64), false))
22+
.isInstanceOf(IllegalArgumentException.class)
23+
.hasMessageContaining("size mismatch");
24+
}
25+
26+
@Test
27+
void construct_nullNames_throwsNullPointerException() {
28+
// Given / When / Then
29+
assertThatThrownBy(() -> new DType.Struct(null, List.of(I64), false))
30+
.isInstanceOf(NullPointerException.class);
31+
}
32+
33+
@Test
34+
void construct_nullTypes_throwsNullPointerException() {
35+
// Given / When / Then
36+
assertThatThrownBy(() -> new DType.Struct(List.of("a"), null, false))
37+
.isInstanceOf(NullPointerException.class);
38+
}
39+
40+
@Test
41+
void construct_duplicateNames_isLegalInMemory() {
42+
// Given — duplicate names, mirroring Rust StructFields (uniqueness is enforced at the
43+
// file boundary by the writer and parser, not by the in-memory type)
44+
// When
45+
DType.Struct result = new DType.Struct(List.of("dup", "dup"), List.of(I64, I64), false);
46+
47+
// Then
48+
assertThat(result.fieldNames()).containsExactly("dup", "dup");
49+
}
50+
}

docs/compatibility.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ resolves only the standalone decoders in `reader`; no encoder class is loaded.
3535
| `vortex.onpair` experimental string encoding | Rust 0.74.0 | ❌ Not registered. Files using it fail to decode unless `Registry.allowUnknown()` is enabled. |
3636
| `vortex.variant` arbitrary nested objects | Rust (`vortex.parquet.variant`) | ⚠️ Java encodes/decodes variant columns of **typed scalar** values (constant / chunked-of-constants core, optional shredded child); Java↔Rust round-trip verified. Arbitrary nested JSON objects and real path-based shredding need the `vortex.parquet.variant` physical encoding — deferred ([ADR 0014](../adr/0014-variant-encoding-strategy.md)). |
3737
| Arrow extension array import affecting Variant shape | Rust 0.74.0 (#8125) | Untested. Re-run integration fixtures against v0.74.0 once published. |
38+
| Duplicate struct field names | Rust writer rejects ("StructLayout must have unique field names"); Rust reader tolerates foreign files (first-match access) | ⚠️ Deliberate divergence on read: Java rejects such files with `VortexException("duplicate field name in file schema")` instead of tolerating them — the name-keyed `Chunk` API cannot represent both columns, and silent column loss is worse than a loud failure on a file the reference writer refuses to produce. Java's writer mirrors the Rust writer's rejection. Empty column name `""` is legal on both sides (pinned by `ColumnNameEdgeCasesIntegrationTest`). |
3839

3940
## Encodings
4041

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import java.lang.foreign.MemorySegment;
2222
import java.util.ArrayList;
23+
import java.util.HashSet;
2324
import java.util.List;
2425

2526
final class PostscriptParser {
@@ -238,10 +239,22 @@ private static DType convertDType(io.github.dfa1.vortex.core.fbs.FbsDType fbs, i
238239
case FbsType.FbsBinary -> new DType.Binary(fbs.type(new FbsBinary()).nullable());
239240
case FbsType.FbsStruct_ -> {
240241
var s = fbs.type(new FbsStruct_());
242+
if (s.namesLength() != s.dtypesLength()) {
243+
throw new VortexException("struct names/dtypes length mismatch: "
244+
+ s.namesLength() + " names, " + s.dtypesLength() + " dtypes");
245+
}
241246
var names = new ArrayList<String>(s.namesLength());
242247
var types = new ArrayList<DType>(s.dtypesLength());
248+
var seen = new HashSet<String>();
243249
for (int i = 0; i < s.namesLength(); i++) {
244-
names.add(s.names(i));
250+
String name = s.names(i);
251+
if (!seen.add(name)) {
252+
// Wire contract, enforced by the reference writer ("StructLayout must
253+
// have unique field names"): a duplicate here is a crafted or corrupt
254+
// file, and the name-keyed Chunk API would silently drop a column.
255+
throw new VortexException("duplicate field name in file schema: " + name);
256+
}
257+
names.add(name);
245258
}
246259
for (int i = 0; i < s.dtypesLength(); i++) {
247260
types.add(convertDType(s.dtypes(i), depth + 1));
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
package io.github.dfa1.vortex.reader;
2+
3+
import io.github.dfa1.vortex.core.error.VortexException;
4+
import io.github.dfa1.vortex.core.fbs.FbsArraySpec;
5+
import io.github.dfa1.vortex.core.fbs.FbsBuilder;
6+
import io.github.dfa1.vortex.core.fbs.FbsDType;
7+
import io.github.dfa1.vortex.core.fbs.FbsFooter;
8+
import io.github.dfa1.vortex.core.fbs.FbsLayout;
9+
import io.github.dfa1.vortex.core.fbs.FbsLayoutSpec;
10+
import io.github.dfa1.vortex.core.fbs.FbsNull;
11+
import io.github.dfa1.vortex.core.fbs.FbsStruct_;
12+
import io.github.dfa1.vortex.core.fbs.FbsType;
13+
import org.junit.jupiter.api.Test;
14+
15+
import java.lang.foreign.MemorySegment;
16+
17+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
18+
19+
/// Drives the struct-dtype guards in `PostscriptParser.convertDType` with crafted dtype
20+
/// FlatBuffers through the package-private [PostscriptParser#parseBlobs] — no file needed.
21+
///
22+
/// Both inputs are files the reference writer refuses to produce ("StructLayout must have
23+
/// unique field names") or that no writer produces at all (names/dtypes arity desync), so they
24+
/// only arrive crafted or corrupt — and untrusted input must fail as [VortexException], never
25+
/// flow into the name-keyed Chunk maps where a duplicate silently drops a column.
26+
class PostscriptParserDTypeGuardsTest {
27+
28+
@Test
29+
void convertDType_duplicateStructFieldNames_throwsVortexException() {
30+
// Given — a dtype blob whose root struct declares two fields named "dup"
31+
MemorySegment dtype = structDType(new String[]{"dup", "dup"}, 2);
32+
33+
// When / Then
34+
assertThatThrownBy(() -> PostscriptParser.parseBlobs(minimalFooter(), flatLayout(), dtype))
35+
.isInstanceOf(VortexException.class)
36+
.hasMessageContaining("duplicate field name in file schema: dup");
37+
}
38+
39+
@Test
40+
void convertDType_structNamesDtypesArityMismatch_throwsVortexException() {
41+
// Given — a dtype blob declaring two field names but only one field type
42+
MemorySegment dtype = structDType(new String[]{"a", "b"}, 1);
43+
44+
// When / Then
45+
assertThatThrownBy(() -> PostscriptParser.parseBlobs(minimalFooter(), flatLayout(), dtype))
46+
.isInstanceOf(VortexException.class)
47+
.hasMessageContaining("names/dtypes length mismatch");
48+
}
49+
50+
// ── FlatBuffer builders ─────────────────────────────────────────────────────
51+
52+
/// Builds a struct dtype blob with the given field names and `dtypeCount` null-typed fields.
53+
private static MemorySegment structDType(String[] names, int dtypeCount) {
54+
var fbb = new FbsBuilder(256);
55+
int[] fieldOffsets = new int[dtypeCount];
56+
for (int i = 0; i < dtypeCount; i++) {
57+
int nul = FbsNull.createFbsNull(fbb);
58+
fieldOffsets[i] = FbsDType.createFbsDType(fbb, FbsType.FbsNull, nul);
59+
}
60+
int[] nameOffsets = new int[names.length];
61+
for (int i = 0; i < names.length; i++) {
62+
nameOffsets[i] = fbb.createString(names[i]);
63+
}
64+
int namesVec = FbsStruct_.createNamesVector(fbb, nameOffsets);
65+
int dtypesVec = FbsStruct_.createDtypesVector(fbb, fieldOffsets);
66+
int struct = FbsStruct_.createFbsStruct_(fbb, namesVec, dtypesVec, false);
67+
int root = FbsDType.createFbsDType(fbb, FbsType.FbsStruct_, struct);
68+
fbb.finish(root);
69+
return fbb.dataSegment();
70+
}
71+
72+
private static MemorySegment minimalFooter() {
73+
var fbb = new FbsBuilder(256);
74+
int asv = FbsFooter.createArraySpecsVector(fbb, new int[]{
75+
FbsArraySpec.createFbsArraySpec(fbb, fbb.createString("vortex.primitive"))});
76+
int lsv = FbsFooter.createLayoutSpecsVector(fbb, new int[]{
77+
FbsLayoutSpec.createFbsLayoutSpec(fbb, fbb.createString("vortex.flat"))});
78+
FbsFooter.startSegmentSpecsVector(fbb, 0);
79+
int ssv = fbb.endVector();
80+
int footOff = FbsFooter.createFbsFooter(fbb, asv, lsv, ssv, 0, 0);
81+
fbb.finish(footOff);
82+
return fbb.dataSegment();
83+
}
84+
85+
private static MemorySegment flatLayout() {
86+
var fbb = new FbsBuilder(128);
87+
int segV = FbsLayout.createSegmentsVector(fbb, new long[]{0});
88+
int off = FbsLayout.createFbsLayout(fbb, 0, 1L, 0, 0, segV);
89+
FbsLayout.finishFbsLayoutBuffer(fbb, off);
90+
return fbb.dataSegment();
91+
}
92+
}

0 commit comments

Comments
 (0)