Skip to content

Commit f371d0c

Browse files
dfa1claude
andcommitted
fix: accept blank column names on read and write (#255)
ColumnName now enforces a symmetric policy: non-null and free of control characters. Blank names ("", " ", …) are wire-legal and produced by the Rust reference (uci-electricityloaddiagrams20112014 has one at field index 0), so they are now accepted on both paths. Control characters remain rejected on both paths; PostscriptParser wraps the rejection as VortexException with field-index context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 603700c commit f371d0c

9 files changed

Lines changed: 72 additions & 60 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1313

1414
### Fixed
1515

16+
- Blank column names are now accepted on both the read and write paths. The Rust reference legitimately produces them (e.g. `uci-electricityloaddiagrams20112014` has one at field index 0). `ColumnName` now enforces a symmetric policy: non-null and free of control characters (`\n`, `\t`, U+0000, …); blank names such as `""` or `" "` pass through on both paths. ([#255](https://github.com/dfa1/vortex-java/issues/255))
1617
- `VarBinArray.DictMode` reads dict-value offsets in a ptype-uniform loop: the I32 fast path eliminates the per-row `switch(dictValOffPType)` that blocked C2 vectorization (introduced by #215). ([#243](https://github.com/dfa1/vortex-java/issues/243))
1718
- `ConstantEncodingDecoder` now returns `NullArray` for null-scalar constants (proto `null_value` tag); previously decoded as `0`/`false`, silently corrupting nullable columns that the Rust writer encodes as an all-null constant. ([#246](https://github.com/dfa1/vortex-java/issues/246))
1819
- `ScanIterator.sliceArray` now handles `NullArray`; previously threw on Rust-written files where a null-typed column used a coarser chunk grid than the other columns. ([#247](https://github.com/dfa1/vortex-java/issues/247))

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

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,28 +3,26 @@
33
import java.util.Objects;
44
import java.util.Optional;
55

6-
/// A column (struct field) name that satisfies vortex-java's name policy: non-blank and free
7-
/// of control characters.
6+
/// A non-null typed wrapper around a column (struct field) name, free of control characters.
87
///
9-
/// The wire format itself accepts almost any UTF-8 string — including `""`, whitespace-only
10-
/// names, and control characters — but those are footguns this implementation refuses on both
11-
/// sides of the file boundary (the writer never emits them, the reader rejects files carrying
12-
/// them). A `ColumnName` certifies at construction that a name passed that policy, so code
13-
/// holding one never re-validates. Printable names of any shape remain legal: `"$$$$$"`,
14-
/// names with interior spaces, and emoji all round-trip against the reference implementation.
8+
/// The wire format accepts almost any UTF-8 string — including `""`, whitespace-only names,
9+
/// and control characters. vortex-java accepts blank names on both the read and write paths
10+
/// (the Rust reference produces them and they are wire-legal), but still rejects control
11+
/// characters because they silently break CSV, JSON, SQL identifiers, and other downstream
12+
/// consumers. Printable names of any shape remain legal: `""`, `"$$$$$"`, names with interior
13+
/// spaces, and emoji all round-trip against the reference implementation.
1514
///
16-
/// The policy checks live in [#violation(String)] — the single source of truth used by the
17-
/// schema builder, the writer, and the file parser (which wraps violations in its own
18-
/// file-context error).
15+
/// The policy check lives in [#violation(String)] — the single source of truth used by the
16+
/// schema builder, the writer, and the file parser.
1917
///
20-
/// @param value the validated name; use it wherever a raw column-name string is required
18+
/// @param value the validated name; non-null and free of control characters
2119
public record ColumnName(String value) implements Comparable<ColumnName> {
2220

23-
/// Validates the name against the policy.
21+
/// Validates the name against the policy (non-null, no control characters).
2422
///
2523
/// @param value the column name
2624
/// @throws NullPointerException if `value` is `null`
27-
/// @throws IllegalArgumentException if the name violates the policy ([#violation(String)])
25+
/// @throws IllegalArgumentException if the name contains a control character
2826
public ColumnName {
2927
Objects.requireNonNull(value, "value");
3028
Optional<String> violation = violation(value);
@@ -33,12 +31,12 @@ public record ColumnName(String value) implements Comparable<ColumnName> {
3331
}
3432
}
3533

36-
/// Creates a validated column name.
34+
/// Creates a validated column name — identical to the compact constructor.
3735
///
3836
/// @param value the column name
3937
/// @return the validated [ColumnName]
4038
/// @throws NullPointerException if `value` is `null`
41-
/// @throws IllegalArgumentException if the name violates the policy ([#violation(String)])
39+
/// @throws IllegalArgumentException if the name contains a control character
4240
public static ColumnName of(String value) {
4341
return new ColumnName(value);
4442
}
@@ -51,9 +49,6 @@ public static ColumnName of(String value) {
5149
/// @param name the raw column name to check (must be non-`null`)
5250
/// @return the policy violation, or empty if `name` is a valid column name
5351
public static Optional<String> violation(String name) {
54-
if (name.isBlank()) {
55-
return Optional.of("blank field name (wire-legal, but a footgun vortex-java refuses to write)");
56-
}
5752
for (int i = 0; i < name.length(); i++) {
5853
if (Character.isISOControl(name.charAt(i))) {
5954
return Optional.of("field name contains control character U+%04X"

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -164,17 +164,15 @@ private StructBuilder() {
164164

165165
/// Adds a named field to the struct under construction.
166166
///
167-
/// Field names follow the [ColumnName] policy, stricter than the wire format on
168-
/// purpose: blank names and control characters are rejected as footguns, on both the
169-
/// write and the read side — same spirit as a JSON library refusing a `""` key it
170-
/// could technically parse.
167+
/// Delegates to [ColumnName#of(String)], which enforces the policy: names must be
168+
/// non-null and free of control characters. Blank names are wire-legal and accepted.
171169
///
172-
/// @param name the field name; non-`null`, non-blank, no control characters, not
173-
/// previously added
170+
/// @param name the field name; non-`null`, no control characters, not previously added
174171
/// @param type the field type
175172
/// @return this builder
176-
/// @throws IllegalArgumentException if `name` is blank, contains a control character,
177-
/// or duplicates a previously added field
173+
/// @throws NullPointerException if `name` is `null`
174+
/// @throws IllegalArgumentException if `name` contains a control character or duplicates
175+
/// a previously added field
178176
public StructBuilder field(String name, DType type) {
179177
return field(ColumnName.of(name), type);
180178
}
Binary file not shown.

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

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,24 @@ void builder_isNotReusable_afterMutation_byField() {
9191
}
9292

9393
@org.junit.jupiter.params.ParameterizedTest
94-
@org.junit.jupiter.params.provider.ValueSource(strings = {"", " ", " ", "a\nb", "nul\u0000here"})
95-
void field_footgunName_throwsIllegalArgumentException(String name) {
96-
// Given the friendly path, which enforces the write-side name policy up front:
97-
// blank and control-character names are wire-legal footguns vortex-java refuses to write
94+
@org.junit.jupiter.params.provider.ValueSource(strings = {"a\nb", "nul\u0000here"})
95+
void field_controlCharName_throwsIllegalArgumentException(String name) {
96+
// Given — control characters are rejected; blank names are wire-legal and allowed
9897
var builder = DType.structBuilder();
9998
// When / Then
10099
org.assertj.core.api.Assertions.assertThatThrownBy(() -> builder.field(name, DType.I64))
101100
.isInstanceOf(IllegalArgumentException.class);
102101
}
102+
103+
@org.junit.jupiter.params.ParameterizedTest
104+
@org.junit.jupiter.params.provider.ValueSource(strings = {"", " ", " "})
105+
void field_blankName_succeeds(String name) {
106+
// Given — blank names are wire-legal; the Rust reference produces them
107+
// When
108+
DType.Struct result = DType.structBuilder().field(name, DType.I64).build();
109+
110+
// Then
111+
org.assertj.core.api.Assertions.assertThat(result.fieldNames())
112+
.containsExactly(ColumnName.of(name));
113+
}
103114
}

integration/src/test/resources/raincloud/expected-status.csv

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ uci-default-of-credit-card-clients,ok
208208
uci-diabetes,ok
209209
uci-diabetes-130-us-hospitals,ok
210210
uci-dry-bean-dataset,ok
211-
uci-electricityloaddiagrams20112014,gap:255
211+
uci-electricityloaddiagrams20112014,ok
212212
uci-energy-efficiency,ok
213213
uci-estimation-of-obesity-levels,ok
214214
uci-forest-fires,ok

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -278,18 +278,16 @@ private static DType convertDType(io.github.dfa1.vortex.core.fbs.FbsDType fbs, i
278278
// file, and the name-keyed Chunk API would silently drop a column.
279279
throw new VortexException("duplicate field name in file schema: " + name);
280280
}
281-
// Same strict name policy as the write side (ColumnName is the single
282-
// source of truth): blank and control-character names are wire-legal but
283-
// almost certainly a bug in the producing pipeline — reject with a message
284-
// that says so rather than propagate unusable names into name-keyed APIs
285-
// and SQL identifiers.
281+
// Blank names are wire-legal and produced by the Rust reference (e.g. an
282+
// empty name at field index 0); ColumnName accepts them. Control characters
283+
// are rejected on both the read and write paths — wrap as VortexException
284+
// here so the caller sees file context rather than a raw IllegalArgumentException.
286285
final int fieldIndex = i;
287286
io.github.dfa1.vortex.core.model.ColumnName.violation(name).ifPresent(reason -> {
288-
throw new VortexException("invalid field name in file schema (field index "
289-
+ fieldIndex + "): " + reason
290-
+ " — likely a bug in the pipeline that produced this file");
287+
throw new io.github.dfa1.vortex.core.error.VortexException(
288+
"invalid field name at field index " + fieldIndex + " in file schema: " + reason);
291289
});
292-
names.add(io.github.dfa1.vortex.core.model.ColumnName.of(name));
290+
names.add(new io.github.dfa1.vortex.core.model.ColumnName(name));
293291
}
294292
for (int i = 0; i < s.dtypesLength(); i++) {
295293
types.add(convertDType(s.dtypes(i), depth + 1));

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

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package io.github.dfa1.vortex.reader;
22

33
import io.github.dfa1.vortex.core.error.VortexException;
4+
import io.github.dfa1.vortex.core.model.ColumnName;
5+
import io.github.dfa1.vortex.core.model.DType;
46
import io.github.dfa1.vortex.core.fbs.FbsArraySpec;
57
import io.github.dfa1.vortex.core.fbs.FbsBuilder;
68
import io.github.dfa1.vortex.core.fbs.FbsDType;
@@ -14,15 +16,19 @@
1416

1517
import java.lang.foreign.MemorySegment;
1618

19+
import static org.assertj.core.api.Assertions.assertThat;
1720
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1821

1922
/// Drives the struct-dtype guards in `PostscriptParser.convertDType` with crafted dtype
2023
/// FlatBuffers through the package-private [PostscriptParser#parseBlobs] — no file needed.
2124
///
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.
25+
/// The duplicate-name and arity-desync inputs are files the reference writer refuses to produce
26+
/// ("StructLayout must have unique field names") or that no writer produces at all, so they only
27+
/// arrive crafted or corrupt — untrusted input must fail as [VortexException], never flow into
28+
/// the name-keyed Chunk maps where a duplicate silently drops a column. Blank names are
29+
/// wire-legal and accepted on both read and write paths. Control characters are rejected on
30+
/// both paths (they break CSV/JSON/SQL downstream); the read path wraps the error as a
31+
/// [VortexException] with field-index context rather than leaking a raw IllegalArgumentException.
2632
class PostscriptParserDTypeGuardsTest {
2733

2834
@Test
@@ -53,32 +59,35 @@ void convertDType_structNamesDtypesArityMismatch_throwsVortexException() {
5359

5460
@org.junit.jupiter.params.ParameterizedTest
5561
@org.junit.jupiter.params.provider.ValueSource(strings = {"", " ", " "})
56-
void convertDType_blankFieldName_throwsVortexException(String name) {
57-
// Given — a file schema carrying a blank field name (wire-legal; the Rust writer can
58-
// produce it) — rejected by policy with a message pointing at the producing pipeline
62+
void convertDType_blankFieldName_isAccepted(String name) {
63+
// Given — a file schema carrying a blank field name (wire-legal; the reference
64+
// implementation produces it, e.g. an empty name at field index 0 in
65+
// uci-electricityloaddiagrams20112014). The read path must not reject it (#255).
5966
MemorySegment dtype = structDType(new String[]{name}, 1);
6067

61-
// When / Then — footer/layout fixtures hoisted so only the subject call is in the lambda
68+
// When
6269
MemorySegment footer = minimalFooter();
6370
MemorySegment layout = flatLayout();
64-
assertThatThrownBy(() -> PostscriptParser.parseBlobs(footer, layout, dtype))
65-
.isInstanceOf(VortexException.class)
66-
.hasMessageContaining("invalid field name in file schema")
67-
.hasMessageContaining("blank field name");
71+
PostscriptParser.ParsedFile result = PostscriptParser.parseBlobs(footer, layout, dtype);
72+
73+
// Then — the blank name round-trips into the parsed struct dtype verbatim
74+
assertThat(result.dtype()).isInstanceOfSatisfying(DType.Struct.class, struct ->
75+
assertThat(struct.fieldNames()).containsExactly(new ColumnName(name)));
6876
}
6977

7078
@Test
7179
void convertDType_controlCharacterFieldName_throwsVortexException() {
72-
// Given — a newline inside a file schema's field name: wire-legal, but it poisons
73-
// SQL/CSV/log renderings downstream, so the reader rejects it like the writer does
80+
// Given — control characters in column names break downstream consumers (CSV/JSON/SQL);
81+
// ColumnName rejects them on both the read and write paths. The read path wraps the
82+
// rejection as VortexException with field-index context (#255).
7483
MemorySegment dtype = structDType(new String[]{"a\nb"}, 1);
7584

7685
// When / Then — footer/layout fixtures hoisted so only the subject call is in the lambda
7786
MemorySegment footer = minimalFooter();
7887
MemorySegment layout = flatLayout();
7988
assertThatThrownBy(() -> PostscriptParser.parseBlobs(footer, layout, dtype))
8089
.isInstanceOf(VortexException.class)
81-
.hasMessageContaining("control character U+000A");
90+
.hasMessageContaining("U+000A");
8291
}
8392

8493
@Test

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,10 +117,10 @@ void schema_fieldNameWithNulByte_isUnbuildable() {
117117
}
118118

119119
@org.junit.jupiter.params.ParameterizedTest
120-
@org.junit.jupiter.params.provider.ValueSource(strings = {"", " ", " ", "a\nb", "tab\there"})
121-
void schema_footgunFieldName_isUnbuildable(String name) {
122-
// Given a blank or control-character field name — wire-legal footguns. With ColumnName-
123-
// typed fieldNames the schema can't hold one; the write-side strictness is now structural.
120+
@org.junit.jupiter.params.provider.ValueSource(strings = {"a\nb", "tab\there"})
121+
void schema_controlCharFieldName_isUnbuildable(String name) {
122+
// Given a control-character field name — rejected by ColumnName on both read and write.
123+
// Blank names are wire-legal and accepted; only control chars break downstream consumers.
124124
// When / Then
125125
assertThatThrownBy(() -> new DType.Struct(
126126
List.of(ColumnName.of(name)),

0 commit comments

Comments
 (0)