Skip to content

Commit c993a35

Browse files
dfa1claude
andcommitted
feat(core,reader,writer): strict field-name policy both ways
Wire-legal is a floor, not a policy: like a JSON library refusing to write the "" key it could technically parse, vortex-java now rejects footgun field names on BOTH sides of the boundary. - StructBuilder.field and VortexWriter reject blank names and control characters (IllegalArgumentException; NUL additionally SIGABRTs the reference toolchain's Arrow FFI, so it must never be written). - PostscriptParser rejects files carrying blank or control character field names (VortexException naming the producing pipeline as the likely bug) — a loud, actionable failure beats propagating unusable names into name-keyed APIs and SQL identifiers. - Printable names of any shape ($$$$$, spaces inside, emoji) remain legal and round-trip intact both directions (measured against vortex-jni; pinned by ColumnNameEdgeCasesIntegrationTest, which also pins that a Rust-written "" file is rejected by policy). Deliberate divergence documented in docs/compatibility.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent fa53ce3 commit c993a35

8 files changed

Lines changed: 129 additions & 103 deletions

File tree

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,28 @@ private StructBuilder() {
164164

165165
/// Adds a named field to the struct under construction.
166166
///
167-
/// @param name the field name; must be non-`null` and not previously added
167+
/// Field names follow the write-side policy, stricter than the wire format on purpose:
168+
/// blank names and control characters are rejected as footguns (a foreign file may
169+
/// carry them and the reader tolerates it, but vortex-java refuses to produce them —
170+
/// same spirit as a JSON library refusing to write a `""` key it would happily parse).
171+
///
172+
/// @param name the field name; non-`null`, non-blank, no control characters, not
173+
/// previously added
168174
/// @param type the field type
169175
/// @return this builder
170-
/// @throws IllegalArgumentException if `name` duplicates a previously added field
176+
/// @throws IllegalArgumentException if `name` is blank, contains a control character,
177+
/// or duplicates a previously added field
171178
public StructBuilder field(String name, DType type) {
179+
if (name.isBlank()) {
180+
throw new IllegalArgumentException(
181+
"blank field name (wire-legal, but a footgun vortex-java refuses to write)");
182+
}
183+
for (int i = 0; i < name.length(); i++) {
184+
if (Character.isISOControl(name.charAt(i))) {
185+
throw new IllegalArgumentException(
186+
"field name contains control character U+%04X".formatted((int) name.charAt(i)));
187+
}
188+
}
172189
if (fields.putIfAbsent(name, type) != null) {
173190
throw new IllegalArgumentException("duplicate field name: " + name);
174191
}

core/src/test/java/io/github/dfa1/vortex/core/model/DTypeStructBuilderTest.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,4 +88,14 @@ void builder_isNotReusable_afterMutation_byField() {
8888
assertThat(resultX.fieldNames()).containsExactly("x");
8989
assertThat(resultY.fieldNames()).containsExactly("y");
9090
}
91+
92+
@org.junit.jupiter.params.ParameterizedTest
93+
@org.junit.jupiter.params.provider.ValueSource(strings = {"", " ", " ", "a\nb", "nul\u0000here"})
94+
void field_footgunName_throwsIllegalArgumentException(String name) {
95+
// Given / When / Then — the friendly path enforces the write-side name policy up front:
96+
// blank and control-character names are wire-legal footguns vortex-java refuses to write
97+
org.assertj.core.api.Assertions.assertThatThrownBy(
98+
() -> DType.structBuilder().field(name, DType.I64))
99+
.isInstanceOf(IllegalArgumentException.class);
100+
}
91101
}

docs/compatibility.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ 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`). |
39-
| NUL (`U+0000`) in a field name | Aborts the Rust toolchain: Arrow FFI schema export hits a panic-cannot-unwind in `arrow-rs` (`ffi_stream::get_schema`) and SIGABRTs the process (measured against vortex-jni 0.75.0) | ⚠️ Java's writer rejects NUL in field names (`IllegalArgumentException`) so we never emit a file the canonical reader dies on; Java's reader tolerates such files (pure-Java path, no C strings). All other names are opaque UTF-8 and round-trip intact both directions — including `""`, whitespace-only, `$`-runs, newlines, and emoji (measured). |
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. |
39+
| Blank / control-character field names | Wire-legal; the Rust writer produces `""` and whitespace-only names. NUL (`U+0000`) additionally aborts the Rust toolchain: Arrow FFI schema export hits a panic-cannot-unwind in `arrow-rs` (`ffi_stream::get_schema`) and SIGABRTs the process (measured against vortex-jni 0.75.0) | ⚠️ Deliberate strictness BOTH ways: vortex-java's writer refuses blank and control-character field names (`IllegalArgumentException`), and its reader rejects files carrying them (`VortexException` naming the producing pipeline as the likely bug) — the JSON-`""`-key principle: wire-legal is a floor, not a policy. Printable names of any shape (`$`-runs, spaces inside, emoji) are legal and round-trip intact both directions (measured; pinned by `ColumnNameEdgeCasesIntegrationTest`). |
4040

4141
## Encodings
4242

Lines changed: 21 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -1,51 +1,43 @@
11
package io.github.dfa1.vortex.integration;
22

3-
import dev.vortex.api.DataSource;
4-
import dev.vortex.api.Scan;
5-
import dev.vortex.api.ScanOptions;
63
import dev.vortex.api.Session;
74
import dev.vortex.api.VortexWriter;
85
import dev.vortex.arrow.ArrowAllocation;
96
import dev.vortex.jni.NativeLoader;
10-
import io.github.dfa1.vortex.core.model.DType;
11-
import io.github.dfa1.vortex.core.model.PType;
7+
import io.github.dfa1.vortex.core.error.VortexException;
128
import io.github.dfa1.vortex.reader.VortexReader;
13-
import io.github.dfa1.vortex.reader.array.LongArray;
149
import org.apache.arrow.c.ArrowArray;
1510
import org.apache.arrow.c.ArrowSchema;
1611
import org.apache.arrow.c.Data;
1712
import org.apache.arrow.memory.BufferAllocator;
1813
import org.apache.arrow.vector.BigIntVector;
1914
import org.apache.arrow.vector.VectorSchemaRoot;
20-
import org.apache.arrow.vector.ipc.ArrowReader;
2115
import org.apache.arrow.vector.types.pojo.ArrowType;
2216
import org.apache.arrow.vector.types.pojo.Field;
2317
import org.apache.arrow.vector.types.pojo.Schema;
2418
import org.junit.jupiter.api.Test;
2519
import org.junit.jupiter.api.io.TempDir;
2620

2721
import java.io.IOException;
28-
import java.nio.channels.FileChannel;
2922
import java.nio.file.Path;
30-
import java.nio.file.StandardOpenOption;
31-
import java.util.ArrayList;
3223
import java.util.HashMap;
3324
import java.util.List;
34-
import java.util.Map;
3525

3626
import static org.assertj.core.api.Assertions.assertThat;
3727
import static org.assertj.core.api.Assertions.catchThrowable;
3828

3929
/// Cross-compatibility for column-name edge cases nobody advertises, measured against the
4030
/// Rust (JNI) reference:
4131
///
42-
/// - `""` is a legal column name: round-trips in BOTH directions (Rust writes/Java reads and
43-
/// Java writes/Rust reads).
32+
/// - Blank names (`""`, whitespace-only) are wire-legal — the Rust writer produces them — but
33+
/// vortex-java refuses them BOTH ways by policy: the writer never emits them and the reader
34+
/// rejects files carrying them with a message pointing at the producing pipeline. Stricter
35+
/// than the wire on purpose, like a JSON library refusing a `""` key it could technically
36+
/// parse (see `VortexWriterTest` / `DTypeStructBuilderTest` / `PostscriptParserDTypeGuardsTest`).
4437
/// - Duplicate field names are legal in Rust's in-memory `StructFields`
4538
/// (`vortex-array/src/dtype/struct_.rs`, first-match name access) but REJECTED by its file
4639
/// 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".
40+
/// enforce; our reader rejects such files too.
4941
class ColumnNameEdgeCasesIntegrationTest {
5042

5143
private static final Session SESSION = Session.create();
@@ -56,45 +48,28 @@ class ColumnNameEdgeCasesIntegrationTest {
5648
}
5749

5850
@Test
59-
void jniWritesEmptyColumnName_javaReadsIt(@TempDir Path tmp) throws IOException {
60-
// Given — the Rust (JNI) writer produces a file whose first column is named ""
51+
void jniWritesEmptyColumnName_javaRejectsItByPolicy(@TempDir Path tmp) throws IOException {
52+
// Given — the Rust (JNI) writer legitimately produces a file whose first column is
53+
// named "" (the wire format permits it)
6154
Path file = tmp.resolve("jni_empty_name.vortex");
6255
Schema schema = new Schema(List.of(
6356
Field.notNullable("", new ArrowType.Int(64, true)),
6457
Field.notNullable("x", new ArrowType.Int(64, true))));
6558
writeJni(file, schema, new long[][]{{1, 2}, {30, 40}});
6659

6760
// 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);
61+
Throwable result = catchThrowable(() -> {
62+
try (var reader = VortexReader.open(file)) {
63+
assertThat(reader).isNotNull();
64+
}
65+
});
9466

95-
// Then — the reference reader accepts the file and reports the empty-named field
96-
assertThat(rows).isEqualTo(2);
97-
assertThat(result).containsExactly("", "x");
67+
// Then — deliberate strictness beyond the wire: a blank name is almost certainly a bug
68+
// in the producing pipeline, and rejecting it loudly beats propagating an unusable name
69+
// into name-keyed APIs and SQL identifiers
70+
assertThat(result)
71+
.isInstanceOf(VortexException.class)
72+
.hasMessageContaining("blank field name in file schema");
9873
}
9974

10075
@Test
@@ -140,48 +115,4 @@ private static void writeJni(Path file, Schema schema, long[][] columns) throws
140115
}
141116
}
142117
}
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-
}
187118
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,22 @@ private static DType convertDType(io.github.dfa1.vortex.core.fbs.FbsDType fbs, i
254254
// file, and the name-keyed Chunk API would silently drop a column.
255255
throw new VortexException("duplicate field name in file schema: " + name);
256256
}
257+
// Same strict name policy as the write side: blank and control-character
258+
// names are wire-legal but almost certainly a bug in the producing
259+
// pipeline — reject with a message that says so rather than propagate
260+
// unusable names into name-keyed APIs and SQL identifiers.
261+
if (name.isBlank()) {
262+
throw new VortexException("blank field name in file schema (field index "
263+
+ i + "): wire-legal, but rejected by policy — likely a bug in "
264+
+ "the pipeline that produced this file");
265+
}
266+
for (int c = 0; c < name.length(); c++) {
267+
if (Character.isISOControl(name.charAt(c))) {
268+
throw new VortexException(
269+
"field name in file schema contains control character U+%04X (field index %d)"
270+
.formatted((int) name.charAt(c), i));
271+
}
272+
}
257273
names.add(name);
258274
}
259275
for (int i = 0; i < s.dtypesLength(); i++) {

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,31 @@ void convertDType_structNamesDtypesArityMismatch_throwsVortexException() {
4747
.hasMessageContaining("names/dtypes length mismatch");
4848
}
4949

50+
@org.junit.jupiter.params.ParameterizedTest
51+
@org.junit.jupiter.params.provider.ValueSource(strings = {"", " ", " "})
52+
void convertDType_blankFieldName_throwsVortexException(String name) {
53+
// Given — a file schema carrying a blank field name (wire-legal; the Rust writer can
54+
// produce it) — rejected by policy with a message pointing at the producing pipeline
55+
MemorySegment dtype = structDType(new String[]{name}, 1);
56+
57+
// When / Then
58+
assertThatThrownBy(() -> PostscriptParser.parseBlobs(minimalFooter(), flatLayout(), dtype))
59+
.isInstanceOf(VortexException.class)
60+
.hasMessageContaining("blank field name in file schema");
61+
}
62+
63+
@Test
64+
void convertDType_controlCharacterFieldName_throwsVortexException() {
65+
// Given — a newline inside a file schema's field name: wire-legal, but it poisons
66+
// SQL/CSV/log renderings downstream, so the reader rejects it like the writer does
67+
MemorySegment dtype = structDType(new String[]{"a\nb"}, 1);
68+
69+
// When / Then
70+
assertThatThrownBy(() -> PostscriptParser.parseBlobs(minimalFooter(), flatLayout(), dtype))
71+
.isInstanceOf(VortexException.class)
72+
.hasMessageContaining("control character U+000A");
73+
}
74+
5075
// ── FlatBuffer builders ─────────────────────────────────────────────────────
5176

5277
/// Builds a struct dtype blob with the given field names and `dtypeCount` null-typed fields.

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,17 +135,26 @@ private VortexWriter(
135135
// names"): duplicate-name schemas are constructible via the DType.Struct record (only
136136
// StructBuilder validates), so guard here — colChunks below is name-keyed and would
137137
// silently collapse the duplicates anyway.
138+
// Write-side name policy — stricter than the wire format on purpose (a JSON parser
139+
// accepts a "" key; a good JSON library still refuses to encourage writing one). The
140+
// reader stays tolerant: foreign files with blank or control-character names are legal
141+
// and must open. Mirrored in DType.StructBuilder.field for earlier, friendlier failure.
138142
var uniqueNames = new java.util.HashSet<String>();
139143
for (String name : schema.fieldNames()) {
140144
if (!uniqueNames.add(name)) {
141145
throw new IllegalArgumentException("duplicate field name: " + name);
142146
}
143-
// A NUL byte in a field name aborts the reference toolchain's Arrow FFI export
144-
// (panic-cannot-unwind in arrow-rs ffi_stream::get_schema, SIGABRT — measured
145-
// 2026-07-04): never emit a file the canonical reader dies on. Everything else,
146-
// including "" and whitespace-only names, is legal and round-trips.
147-
if (name.indexOf('\u0000') >= 0) {
148-
throw new IllegalArgumentException("field name contains NUL (U+0000)");
147+
if (name.isBlank()) {
148+
throw new IllegalArgumentException(
149+
"blank field name (wire-legal, but a footgun vortex-java refuses to write)");
150+
}
151+
for (int i = 0; i < name.length(); i++) {
152+
// NUL aborts the reference toolchain's Arrow FFI export (SIGABRT in arrow-rs,
153+
// measured 2026-07-04); other control characters poison SQL/CSV/log renderings.
154+
if (Character.isISOControl(name.charAt(i))) {
155+
throw new IllegalArgumentException(
156+
"field name contains control character U+%04X".formatted((int) name.charAt(i)));
157+
}
149158
}
150159
}
151160
this.channel = channel;

0 commit comments

Comments
 (0)