Skip to content

Commit ea01579

Browse files
dfa1claude
andcommitted
refactor: FsstEncodingEncoder reuses PTypeIO.set instead of reimplementing it
The private writeUnsigned helper duplicated core.io.PTypeIO.set, the canonical narrow-ptype write helper (MethodHandle-based, byte-offset addressed, already used by every other encoder). Its default branch throwing VortexException was dead: narrowestUnsigned only ever returns U8/U16/U32, all of which PTypeIO.set handles with identical little-endian layout and truncation-on-narrowing. Deleted writeUnsigned and the now-unused VortexException import; the two call sites now pass idx * ptype.byteSize() as the byte offset, matching the established PTypeIO.set idiom. Also close a round-trip coverage gap this refactor's own risk surface exposed: every stringArrays() case kept uncompLenPType/codesOffPType in the U8 tier, so the U16 length/offset write path was only checked via metadata assertions, never a full decode. Add a 301-row u16-tier case (one 300-byte row plus 300 single-char rows) that goes through encode_thenDecode_roundtripsAllStrings, so the U16 buffer bytes are read back and verified per row -- a wrong U16 stride now corrupts many rows and fails the assertion. A metadata test pins that both ptypes escalate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 7af2a78 commit ea01579

2 files changed

Lines changed: 64 additions & 22 deletions

File tree

writer/src/main/java/io/github/dfa1/vortex/writer/encode/FsstEncodingEncoder.java

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import io.github.dfa1.vortex.core.model.DType;
44
import io.github.dfa1.vortex.core.model.PType;
55
import io.github.dfa1.vortex.core.model.EncodingId;
6-
import io.github.dfa1.vortex.core.error.VortexException;
6+
import io.github.dfa1.vortex.core.io.PTypeIO;
77
import io.github.dfa1.vortex.core.io.VortexFormat;
88
import io.github.dfa1.vortex.core.proto.ProtoFSSTMetadata;
99

@@ -121,17 +121,19 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) {
121121
PType uncompLenPType = PType.narrowestUnsigned(maxUncompLen);
122122
PType codesOffPType = PType.narrowestUnsigned(totalCompressed);
123123

124-
MemorySegment uncompLenBuf = arena.allocate(Math.max((long) n * uncompLenPType.byteSize(), 1));
124+
long uncompLenBytes = uncompLenPType.byteSize();
125+
MemorySegment uncompLenBuf = arena.allocate(Math.max((long) n * uncompLenBytes, 1));
125126
for (int i = 0; i < n; i++) {
126-
writeUnsigned(uncompLenBuf, uncompLenPType, i, byteArrays[i].length);
127+
PTypeIO.set(uncompLenBuf, i * uncompLenBytes, uncompLenPType, byteArrays[i].length);
127128
}
128129

129-
MemorySegment codesOffBuf = arena.allocate((long) (n + 1) * codesOffPType.byteSize());
130+
long codesOffBytes = codesOffPType.byteSize();
131+
MemorySegment codesOffBuf = arena.allocate((long) (n + 1) * codesOffBytes);
130132
long off = 0;
131-
writeUnsigned(codesOffBuf, codesOffPType, 0, 0);
133+
PTypeIO.set(codesOffBuf, 0, codesOffPType, 0);
132134
for (int i = 0; i < n; i++) {
133135
off += compressed[i].length;
134-
writeUnsigned(codesOffBuf, codesOffPType, i + 1, off);
136+
PTypeIO.set(codesOffBuf, (i + 1) * codesOffBytes, codesOffPType, off);
135137
}
136138

137139
byte[] metaBytes = new ProtoFSSTMetadata(
@@ -152,21 +154,6 @@ public EncodeResult encode(DType dtype, Object data, EncodeContext ctx) {
152154
null, null);
153155
}
154156

155-
/// Writes `value` into `seg` at row `idx`, using `ptype`'s byte width.
156-
///
157-
/// @param seg destination segment
158-
/// @param ptype `U8`, `U16`, or `U32` (whatever [PType#narrowestUnsigned(long)] returned)
159-
/// @param idx row index (not a byte offset)
160-
/// @param value the value to write
161-
private static void writeUnsigned(MemorySegment seg, PType ptype, long idx, long value) {
162-
switch (ptype) {
163-
case U8 -> seg.set(ValueLayout.JAVA_BYTE, idx, (byte) value);
164-
case U16 -> seg.set(VortexFormat.LE_SHORT, idx * 2, (short) value);
165-
case U32 -> seg.set(VortexFormat.LE_INT, idx * 4, (int) value);
166-
default -> throw new VortexException(EncodingId.VORTEX_FSST, "unexpected ptype: " + ptype);
167-
}
168-
}
169-
170157
/// Trains a symbol table by iteratively refining candidate symbols against a bounded sample.
171158
///
172159
/// @param byteArrays the raw byte content of every input string

writer/src/test/java/io/github/dfa1/vortex/writer/encode/FsstEncodingEncoderTest.java

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,10 +73,34 @@ static Stream<Arguments> stringArrays() {
7373
// Non-ASCII multi-byte UTF-8, repeated so symbols can span codepoint boundaries.
7474
Arguments.of("utf8-repeated", repeat("café ☕ münchen 日本語テスト", 30)),
7575
// Exactly-8-byte symbol repeated: confirms the length-8 boundary encodes/decodes.
76-
Arguments.of("exact-8-byte", repeat("ABCDEFGH", 50))
76+
Arguments.of("exact-8-byte", repeat("ABCDEFGH", 50)),
77+
// U16-tier round-trip: forces both uncompLenPType and codesOffPType off the U8
78+
// fast path so the actual U16 buffer bytes are read back and verified end to end,
79+
// not just the metadata ptype. Row 0 is 300 raw bytes (> 255 -> U16 row lengths);
80+
// the six distinct incompressible rows together compress to > 255 bytes
81+
// (> 255 cumulative codes offset -> U16 offsets). Multiple rows mean a wrong
82+
// U16 stride in the length/offset write path shifts more than one value, so the
83+
// per-index round-trip assertion catches an off-by-stride byte-arithmetic bug.
84+
Arguments.of("u16-tier-multi-row", u16TierRows())
7785
);
7886
}
7987

88+
/// One 300-byte row followed by 300 single-character rows. The long first row pushes the
89+
/// maximum raw row length past the U8 range (forcing `U16` uncompressed lengths). Each
90+
/// single-character row compresses to exactly one code byte regardless of how training
91+
/// ranks symbols, so the cumulative codes-offset total is deterministically at least 300 —
92+
/// past the U8 range too (forcing `U16` codes offsets). 301 rows mean a wrong U16 stride in
93+
/// either buffer write shifts many values, so the per-index round-trip assertion catches an
94+
/// off-by-stride byte-arithmetic bug rather than a lone corrupted row that might slip by.
95+
private static String[] u16TierRows() {
96+
String[] rows = new String[301];
97+
rows[0] = "a".repeat(300);
98+
for (int i = 1; i < rows.length; i++) {
99+
rows[i] = "x";
100+
}
101+
return rows;
102+
}
103+
80104
private static String[] repeatCycle(String[] cycle, int times) {
81105
String[] arr = new String[cycle.length * times];
82106
for (int t = 0; t < times; t++) {
@@ -380,5 +404,36 @@ void encode_metadata_codesOffsetsPType_escalates_pastU8Range() throws Exception
380404
assertThat(meta.uncompressed_lengths_ptype().value()).isEqualTo(PType.U8.ordinal());
381405
assertThat(meta.codes_offsets_ptype().value()).isEqualTo(PType.U16.ordinal());
382406
}
407+
408+
@Test
409+
void encode_metadata_bothPTypes_escalate_forU16RoundtripCase() throws Exception {
410+
// Given — the same multi-row data the U16-tier round-trip case uses (Encode
411+
// .stringArrays "u16-tier-multi-row"). This pins the precondition that the round-trip
412+
// actually exercises the U16 write path for both buffers: if a future change made
413+
// either buffer stay in the U8 tier, this fails and the round-trip coverage would
414+
// silently no longer cover U16.
415+
String[] data = u16TierRowsCopy();
416+
417+
// When
418+
EncodeResult result = ENCODER.encode(DTypes.UTF8, data, EncodeTestHelper.testCtx());
419+
var metaSeg = result.rootNode().metadata();
420+
ProtoFSSTMetadata meta = ProtoFSSTMetadata.decode(metaSeg, 0, metaSeg.byteSize());
421+
422+
// Then
423+
assertThat(meta.uncompressed_lengths_ptype().value()).isEqualTo(PType.U16.ordinal());
424+
assertThat(meta.codes_offsets_ptype().value()).isEqualTo(PType.U16.ordinal());
425+
}
426+
427+
// Mirror of Encode.u16TierRows() — the nested classes cannot share a private static helper
428+
// without exposing it, and the intent (deterministic U16-on-both data) is small enough to
429+
// restate here next to the assertion that depends on it.
430+
private static String[] u16TierRowsCopy() {
431+
String[] rows = new String[301];
432+
rows[0] = "a".repeat(300);
433+
for (int i = 1; i < rows.length; i++) {
434+
rows[i] = "x";
435+
}
436+
return rows;
437+
}
383438
}
384439
}

0 commit comments

Comments
 (0)