Skip to content

fix: encoding of Variant object header field-id and offset sizes - #421

Merged
CurtHagenlocher merged 1 commit into
apache:mainfrom
rayokota:fix-variant-obj-header
Aug 22, 2026
Merged

fix: encoding of Variant object header field-id and offset sizes#421
CurtHagenlocher merged 1 commit into
apache:mainfrom
rayokota:fix-variant-obj-header

Conversation

@rayokota

Copy link
Copy Markdown
Contributor

What

VariantEncodingHelper wrote and read the Variant object value header with
field_id_size_minus_one and field_offset_size_minus_one in each other's bit positions.

Per apache/parquet-format VariantEncoding.md, the object value_header — the 6 bits above
the 2 basic-type bits — is laid out as:

                  5   4  3     2 1     0
                +---+---+-------+-------+
value_header    | R |   |       |       |
                +---+---+-------+-------+
                      ^     ^       ^
                      |     |       +-- field_offset_size_minus_one
                      |     +-- field_id_size_minus_one
                      +-- is_large

MakeObjectHeader and ParseObjectHeader had the two 2-bit fields transposed, and the layout
comment above them documented the same transposition — so the block was internally consistent
rather than wrong in one expression.

is_large was already correct. The array header and metadata header helpers were checked and
match 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 (fieldIdSize from the maximum
field ID, offsetSize from the encoded data length), so they diverge routinely: for example an
object 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 is 0x12;
before this change we emitted 0x06, and read 0x12 back as fieldIdSize=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, and
    correct the layout comment. The out parameters were already named correctly, so neither
    call site — VariantValueWriter or VariantObjectReader — needed changes.
  • VariantEncodingHelperTests: add MakeObjectHeaderUsesSpecBitLayout and

Closes #420.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / ParseObjectHeader to place/read offsetSize in value_header bits 0–1 and fieldIdSize in 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
CurtHagenlocher merged commit 1abe8ab into apache:main Aug 22, 2026
14 checks passed

@CurtHagenlocher CurtHagenlocher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! How embarrassing for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Variant object header encodes field_id_size and field_offset_size in swapped bit positions

3 participants