fix(writer): give Fsst/VarBinView/Zstd a byte-safe Binary path - #361
Merged
Conversation
FsstEncodingEncoder, VarBinViewEncodingEncoder, and ZstdEncodingEncoder all cast data straight to String[], so a DType.Binary column (raw bytes, e.g. an embedded audio/image blob) threw ClassCastException if the cascade competition picked one of them. accepts() had been narrowed to Utf8-only as a stopgap. Add VarBinBytes, a small shared helper (mirroring VarBinEncodingEncoder's existing byte[][]-or-String[] handling) that normalizes both to byte[][] — UTF-8 encoding Utf8 rows, passing Binary rows through untouched, with two null-handling variants (substitute empty, or preserve for detect/strip). All three encoders now accept Binary and round-trip byte-for-byte, including non-UTF8 sequences and null entries (values child of a masked/nullable layout). CascadingCompressor previously routed Binary through a first-match findPrimitiveEncoding + spliceResult, not the real sample-and-measure competition it gives Utf8 — so even with accepts() fixed, whichever encoder happened to register first would always win. Binary now joins Utf8 in the same competeAndEncode path, with a byte[][] case added to dataLength/stratifiedSample. MaskedEncodingEncoder's denseValues (substituting a placeholder for null values-child entries before the cascade reads them) now handles byte[][] alongside String[]. Also fixes an adjacent gap this surfaced: VortexWriter's row-count validation (arrayLength) had no byte[][] case, so a non-nullable DType.Binary column could not be written at all — only nullable ones worked, because NullableData carries its own length. Fixes #352 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019NqSD9faDK9WHNektV8ZPH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FsstEncodingEncoder,VarBinViewEncodingEncoder, andZstdEncodingEncodercastdatastraight toString[], so aDType.Binarycolumn threwClassCastExceptionif the cascade competition picked one of them (accepts()had been narrowed to Utf8-only as a stopgap). AddedVarBinBytes, a shared helper normalizing Utf8/Binary input tobyte[][], and gave all three encoders a real byte-safe path.CascadingCompressorroutedBinarythrough a first-matchfindPrimitiveEncodinginstead of the real sample-and-measure competition Utf8 gets, so even withaccepts()fixed there was no actual contest.Binarynow joinsUtf8incompeteAndEncode, withbyte[][]support added todataLength/stratifiedSample.MaskedEncodingEncoder's null-densification now coversbyte[][]too.VortexWriter.arrayLengthhad nobyte[][]case, so a non-nullableDType.Binarycolumn could not be written at all (only nullable ones worked, sinceNullableDatacarries its own length).Fixes #352
Test plan
./mvnw -pl writer -am test— full writer suite green, including new Binary round-trip tests (byte-for-byte, non-UTF8 sequences, null entries) added toFsstEncodingEncoderTest,VarBinViewEncodingEncoderTest,ZstdEncodingEncoderTest, and newVortexWriterTestcases for non-nullable Binary and cascade competition over Binary../mvnw verify— full reactor build green.🤖 Generated with Claude Code
https://claude.ai/code/session_019NqSD9faDK9WHNektV8ZPH