Skip to content

Commit dca815b

Browse files
dfa1claude
dfa1
and
claude
committed
fix(writer): reject duplicate field names; pin name edge cases
The reference file writer enforces "StructLayout must have unique field names" (its in-memory StructFields allows duplicates with first-match access — the uniqueness contract is wire-level). VortexWriter now mirrors the rejection: a duplicate-name schema is constructible via the DType.Struct record (only StructBuilder validates) and previously wrote files the canonical implementation would never produce, with colChunks silently collapsing columns. ColumnNameEdgeCasesIntegrationTest pins the oracle facts: "" is a legal column name round-tripping in both directions (Rust writes / Java reads and Java writes / Rust reads), and the JNI writer's duplicate rejection with its exact error. Reader-side handling of foreign duplicate-name files remains open in TODO.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 6dec8f6 commit dca815b

4 files changed

Lines changed: 234 additions & 12 deletions

File tree

‎TODO.md‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -95,18 +95,23 @@ 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 (BUG + ADR candidate)** — measured 2026-07-04:
99-
`""` and duplicate field names round-trip through our writer/reader (the `DType.Struct` record
100-
constructor validates nothing; only `StructBuilder` rejects duplicates, and the proto decode path
101-
bypasses it — it can also desync `fieldNames`/`fieldTypes` sizes). The Rust reference *documents*
102-
duplicates as legal with **first**-match name resolution (`vortex-array/src/dtype/struct_.rs`),
103-
but our `Chunk`'s `Map<String, Array>` silently drops columns (two `dup` fields → one entry) and
104-
resolves **last**-wins — a legal Rust file loses data in our reader. Also: duplicate-name write
105-
produced two chunks from one `writeChunk` (unexplained, investigate).
106-
Fix direction (ADR): `Chunk` columns become ordered (list/parallel arrays) with name lookup =
107-
first match, mirroring Rust; `columns()` map API rethought. Revisit `ColumnName` as a domain
108-
primitive here — note the honest limit: `""` is wire-legal, so the per-value invariant is thin;
109-
the real invariants are structural (ordered fields, first-match resolution, names/types arity).
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.
110115

111116
## Compute
112117

‎integration/src/test/java/io/github/dfa1/vortex/integration/ColumnNameEdgeCasesIntegrationTest.java‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
package io.github.dfa1.vortex.integration;
2+
3+
import dev.vortex.api.DataSource;
4+
import dev.vortex.api.Scan;
5+
import dev.vortex.api.ScanOptions;
6+
import dev.vortex.api.Session;
7+
import dev.vortex.api.VortexWriter;
8+
import dev.vortex.arrow.ArrowAllocation;
9+
import dev.vortex.jni.NativeLoader;
10+
import io.github.dfa1.vortex.core.model.DType;
11+
import io.github.dfa1.vortex.core.model.PType;
12+
import io.github.dfa1.vortex.reader.VortexReader;
13+
import io.github.dfa1.vortex.reader.array.LongArray;
14+
import org.apache.arrow.c.ArrowArray;
15+
import org.apache.arrow.c.ArrowSchema;
16+
import org.apache.arrow.c.Data;
17+
import org.apache.arrow.memory.BufferAllocator;
18+
import org.apache.arrow.vector.BigIntVector;
19+
import org.apache.arrow.vector.VectorSchemaRoot;
20+
import org.apache.arrow.vector.ipc.ArrowReader;
21+
import org.apache.arrow.vector.types.pojo.ArrowType;
22+
import org.apache.arrow.vector.types.pojo.Field;
23+
import org.apache.arrow.vector.types.pojo.Schema;
24+
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.api.io.TempDir;
26+
27+
import java.io.IOException;
28+
import java.nio.channels.FileChannel;
29+
import java.nio.file.Path;
30+
import java.nio.file.StandardOpenOption;
31+
import java.util.ArrayList;
32+
import java.util.HashMap;
33+
import java.util.List;
34+
import java.util.Map;
35+
36+
import static org.assertj.core.api.Assertions.assertThat;
37+
import static org.assertj.core.api.Assertions.catchThrowable;
38+
39+
/// Cross-compatibility for column-name edge cases nobody advertises, measured against the
40+
/// Rust (JNI) reference:
41+
///
42+
/// - `""` is a legal column name: round-trips in BOTH directions (Rust writes/Java reads and
43+
/// Java writes/Rust reads).
44+
/// - Duplicate field names are legal in Rust's in-memory `StructFields`
45+
/// (`vortex-array/src/dtype/struct_.rs`, first-match name access) but REJECTED by its file
46+
/// writer: "StructLayout must have unique field names" — the wire contract both writers
47+
/// must enforce. Reader-side handling of foreign duplicate-name files is tracked in
48+
/// TODO.md §"Column identity".
49+
class ColumnNameEdgeCasesIntegrationTest {
50+
51+
private static final Session SESSION = Session.create();
52+
private static final BufferAllocator ALLOCATOR = ArrowAllocation.rootAllocator();
53+
54+
static {
55+
NativeLoader.loadJni();
56+
}
57+
58+
@Test
59+
void jniWritesEmptyColumnName_javaReadsIt(@TempDir Path tmp) throws IOException {
60+
// Given — the Rust (JNI) writer produces a file whose first column is named ""
61+
Path file = tmp.resolve("jni_empty_name.vortex");
62+
Schema schema = new Schema(List.of(
63+
Field.notNullable("", new ArrowType.Int(64, true)),
64+
Field.notNullable("x", new ArrowType.Int(64, true))));
65+
writeJni(file, schema, new long[][]{{1, 2}, {30, 40}});
66+
67+
// When
68+
Map<String, long[]> result = javaReadAllLongColumns(file);
69+
70+
// Then — the empty name is a plain map key, values intact
71+
assertThat(result.keySet()).containsExactlyInAnyOrder("", "x");
72+
assertThat(result.get("")).containsExactly(1, 2);
73+
assertThat(result.get("x")).containsExactly(30, 40);
74+
}
75+
76+
@Test
77+
void javaWritesEmptyColumnName_jniReadsIt(@TempDir Path tmp) throws IOException {
78+
// Given — the Java writer produces a file with an empty column name (the Struct record
79+
// constructor performs no name validation; only StructBuilder rejects duplicates)
80+
Path file = tmp.resolve("java_empty_name.vortex");
81+
var dtype = new DType.Struct(
82+
List.of("", "x"),
83+
List.of(new DType.Primitive(PType.I64, false), new DType.Primitive(PType.I64, false)),
84+
false);
85+
try (var ch = FileChannel.open(file, StandardOpenOption.CREATE, StandardOpenOption.WRITE);
86+
var writer = io.github.dfa1.vortex.writer.VortexWriter.create(
87+
ch, dtype, io.github.dfa1.vortex.writer.WriteOptions.defaults())) {
88+
writer.writeChunk(Map.of("", new long[]{1, 2}, "x", new long[]{30, 40}));
89+
}
90+
91+
// When — the Rust (JNI) reader scans it
92+
List<String> result = new ArrayList<>();
93+
long rows = jniReadFieldNames(file, result);
94+
95+
// Then — the reference reader accepts the file and reports the empty-named field
96+
assertThat(rows).isEqualTo(2);
97+
assertThat(result).containsExactly("", "x");
98+
}
99+
100+
@Test
101+
void jniWriterRejectsDuplicateColumnNames(@TempDir Path tmp) {
102+
// Given — a schema with two fields named "dup". Rust's in-memory StructFields documents
103+
// duplicates as legal (first-match name access), but its FILE writer enforces uniqueness:
104+
// "StructLayout must have unique field names". This test pins that wire contract — the
105+
// Java writer must mirror the same rejection (see VortexWriterTest).
106+
Path file = tmp.resolve("jni_dup_names.vortex");
107+
Schema schema = new Schema(List.of(
108+
Field.notNullable("dup", new ArrowType.Int(64, true)),
109+
Field.notNullable("dup", new ArrowType.Int(64, true))));
110+
111+
// When
112+
Throwable result = catchThrowable(() -> writeJni(file, schema, new long[][]{{1, 2}, {30, 40}}));
113+
114+
// Then — the reference writer refuses to produce a duplicate-name file
115+
assertThat(result)
116+
.isInstanceOf(IOException.class)
117+
.hasRootCauseMessage("Vortex Error: Other error: StructLayout must have unique field names");
118+
}
119+
120+
// ── helpers ───────────────────────────────────────────────────────────────
121+
122+
/// Writes one batch through the Rust (JNI) writer: one `long[]` per schema field, in order.
123+
private static void writeJni(Path file, Schema schema, long[][] columns) throws IOException {
124+
String uri = file.toAbsolutePath().toUri().toString();
125+
try (VortexWriter writer = VortexWriter.create(SESSION, uri, schema, new HashMap<>(), ALLOCATOR);
126+
VectorSchemaRoot root = VectorSchemaRoot.create(schema, ALLOCATOR)) {
127+
int n = columns[0].length;
128+
for (int c = 0; c < columns.length; c++) {
129+
BigIntVector vec = (BigIntVector) root.getVector(c);
130+
vec.allocateNew(n);
131+
for (int i = 0; i < n; i++) {
132+
vec.setSafe(i, columns[c][i]);
133+
}
134+
}
135+
root.setRowCount(n);
136+
try (ArrowArray arr = ArrowArray.allocateNew(ALLOCATOR);
137+
ArrowSchema arrowSchema = ArrowSchema.allocateNew(ALLOCATOR)) {
138+
Data.exportVectorSchemaRoot(ALLOCATOR, root, null, arr, arrowSchema);
139+
writer.writeBatch(arr.memoryAddress(), arrowSchema.memoryAddress());
140+
}
141+
}
142+
}
143+
144+
/// Scans the whole file with the Java reader, materializing every column as `long[]`.
145+
private static Map<String, long[]> javaReadAllLongColumns(Path file) throws IOException {
146+
Map<String, long[]> out = new HashMap<>();
147+
try (var reader = VortexReader.open(file);
148+
var iter = reader.scan(io.github.dfa1.vortex.reader.ScanOptions.all())) {
149+
while (iter.hasNext()) {
150+
try (var chunk = iter.next()) {
151+
for (var entry : chunk.columns().entrySet()) {
152+
LongArray col = (LongArray) entry.getValue();
153+
long[] values = new long[(int) col.length()];
154+
for (int i = 0; i < values.length; i++) {
155+
values[i] = col.getLong(i);
156+
}
157+
out.put(entry.getKey(), values);
158+
}
159+
}
160+
}
161+
}
162+
return out;
163+
}
164+
165+
/// Scans the file with the Rust (JNI) reader, returning the row count and collecting the
166+
/// Arrow schema field names.
167+
private static long jniReadFieldNames(Path file, List<String> namesOut) throws IOException {
168+
long rows = 0;
169+
DataSource ds = DataSource.open(SESSION, file.toAbsolutePath().toUri().toString());
170+
Scan scan = ds.scan(ScanOptions.of());
171+
while (scan.hasNext()) {
172+
var partition = scan.next();
173+
try (ArrowReader reader = partition.scanArrow(ALLOCATOR)) {
174+
while (reader.loadNextBatch()) {
175+
var root = reader.getVectorSchemaRoot();
176+
rows += root.getRowCount();
177+
if (namesOut.isEmpty()) {
178+
for (Field field : root.getSchema().getFields()) {
179+
namesOut.add(field.getName());
180+
}
181+
}
182+
}
183+
}
184+
}
185+
return rows;
186+
}
187+
}

‎writer/src/main/java/io/github/dfa1/vortex/writer/VortexWriter.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,16 @@ public final class VortexWriter implements Closeable {
131131
private VortexWriter(
132132
WritableByteChannel channel, DType.Struct schema, WriteOptions options, List<EncodingEncoder> encodings
133133
) {
134+
// Wire contract, enforced by the reference writer ("StructLayout must have unique field
135+
// names"): duplicate-name schemas are constructible via the DType.Struct record (only
136+
// StructBuilder validates), so guard here — colChunks below is name-keyed and would
137+
// silently collapse the duplicates anyway.
138+
var uniqueNames = new java.util.HashSet<String>();
139+
for (String name : schema.fieldNames()) {
140+
if (!uniqueNames.add(name)) {
141+
throw new IllegalArgumentException("duplicate field name: " + name);
142+
}
143+
}
134144
this.channel = channel;
135145
this.schema = schema;
136146
this.options = options;

‎writer/src/test/java/io/github/dfa1/vortex/writer/VortexWriterTest.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,26 @@ void writeSegments_are64ByteAligned(@TempDir Path tmp) throws IOException {
7777
}
7878
}
7979

80+
@Test
81+
void create_duplicateFieldNames_throwsIllegalArgumentException(@TempDir Path tmp) throws IOException {
82+
// Given — a duplicate-name schema built via the DType.Struct record, which validates
83+
// nothing (only StructBuilder rejects duplicates). The reference writer refuses such
84+
// schemas ("StructLayout must have unique field names"), so ours must too — otherwise
85+
// we emit files the canonical implementation would never produce.
86+
var schema = new DType.Struct(
87+
List.of("dup", "dup"),
88+
List.of(new DType.Primitive(PType.I64, false), new DType.Primitive(PType.I64, false)),
89+
false);
90+
Path file = tmp.resolve("dup.vtx");
91+
92+
// When / Then
93+
try (var ch = FileChannel.open(file, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) {
94+
assertThatThrownBy(() -> VortexWriter.create(ch, schema, WriteOptions.defaults()))
95+
.isInstanceOf(IllegalArgumentException.class)
96+
.hasMessage("duplicate field name: dup");
97+
}
98+
}
99+
80100
// ── writeChunk validation ─────────────────────────────────────────────────
81101

82102
@Test

0 commit comments

Comments
 (0)