fix: encoding of Variant object header field-id and offset sizes - #421
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes the Variant object value header bit layout in VariantEncodingHelper so field_id_size_minus_one and field_offset_size_minus_one are encoded/decoded in the spec-defined bit positions, improving interoperability with other Variant implementations (per apache/parquet-format).
Changes:
- Corrects
MakeObjectHeader/ParseObjectHeaderto place/readoffsetSizein value_header bits 0–1 andfieldIdSizein bits 2–3 (and updates the accompanying layout comment). - Adds spec-pinned unit tests that assert literal header bytes (instead of only round-tripping Make↔Parse).
- Adds an asymmetric object test vector (
field_id_size=2,offset_size=1) and reader tests to validate correct field name/value parsing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs | Swaps object header size bitfields to match the Variant spec and updates the inline bit layout comment. |
| test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs | Adds literal-byte tests to catch swapped bitfield regressions (both Make and Parse directions). |
| test/Apache.Arrow.Scalars.Tests/TestVectors.cs | Introduces an asymmetric object vector (Object_Age30_Name_Bob_WideFieldIds) to exercise differing field-id vs offset widths. |
| test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs | Adds reader coverage for the asymmetric vector (field names, values, and TryGetField). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CurtHagenlocher
left a comment
Contributor
There was a problem hiding this comment.
Thanks! How embarrassing for me!
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
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.
What
VariantEncodingHelperwrote and read the Variant object value header withfield_id_size_minus_oneandfield_offset_size_minus_onein each other's bit positions.Per
apache/parquet-formatVariantEncoding.md, the objectvalue_header— the 6 bits abovethe 2 basic-type bits — is laid out as:
MakeObjectHeaderandParseObjectHeaderhad the two 2-bit fields transposed, and the layoutcomment above them documented the same transposition — so the block was internally consistent
rather than wrong in one expression.
is_largewas already correct. The array header and metadata header helpers were checked andmatch the spec. This affects the object header only.
Impact
Reader and writer shared the inverted convention, so arrow-dotnet round-tripped its own output
correctly. The bug was only observable across implementations, and only when
fieldIdSize != offsetSize— when the two are equal, transposing them is a no-op.Those sizes are computed independently in
VariantValueWriter(fieldIdSizefrom the maximumfield ID,
offsetSizefrom the encoded data length), so they diverge routinely: for example anobject drawn from a >255-entry metadata dictionary (2-byte field IDs) whose own field data is
under 256 bytes (1-byte offsets).
For
fieldIdSize=2, offsetSize=1, isLarge=false, the spec-correct header byte is0x12;before this change we emitted
0x06, and read0x12back asfieldIdSize=1, offsetSize=2.Such objects were silently misparsed in both directions — field IDs and offsets read at the
wrong widths, surfacing as garbage field values or out-of-range offsets rather than a clean
error.
Changes
VariantEncodingHelper.MakeObjectHeader/ParseObjectHeader: swap the two shifts, andcorrect the layout comment. The
outparameters were already named correctly, so neithercall site —
VariantValueWriterorVariantObjectReader— needed changes.VariantEncodingHelperTests: addMakeObjectHeaderUsesSpecBitLayoutandCloses #420.