This is an automated email from the ASF dual-hosted git repository.
CurtHagenlocher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-dotnet.git
The following commit(s) were added to refs/heads/main by this push:
new 1abe8ab fix: encoding of Variant object header field-id and offset
sizes (#421)
1abe8ab is described below
commit 1abe8ab88d5ac3f1bb660d610ed4aec6c974e8ad
Author: Robert Yokota <[email protected]>
AuthorDate: Sat Aug 22 06:34:34 2026 -0700
fix: encoding of Variant object header field-id and offset sizes (#421)
### 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.
---
.../Variant/VariantEncodingHelper.cs | 12 +++---
test/Apache.Arrow.Scalars.Tests/TestVectors.cs | 31 ++++++++++++++
.../VariantEncodingHelperTests.cs | 39 ++++++++++++++++++
.../VariantReaderObjectTests.cs | 47 ++++++++++++++++++++++
4 files changed, 123 insertions(+), 6 deletions(-)
diff --git a/src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs
b/src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs
index 6975bfb..5e87c21 100644
--- a/src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs
+++ b/src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs
@@ -104,8 +104,8 @@ namespace Apache.Arrow.Scalars.Variant
// Object value header
// ---------------------------------------------------------------
//
- // Bits 2-3: field_id_size - 1 (0-3 => 1-4 bytes)
- // Bits 4-5: field_offset_size - 1 (0-3 => 1-4 bytes)
+ // Bits 2-3: field_offset_size - 1 (0-3 => 1-4 bytes)
+ // Bits 4-5: field_id_size - 1 (0-3 => 1-4 bytes)
// Bit 6: is_large (0 = 1-byte num_fields, 1 = 4-byte num_fields)
// Bit 7: unused (must be 0)
@@ -118,8 +118,8 @@ namespace Apache.Arrow.Scalars.Variant
public static byte MakeObjectHeader(int fieldIdSize, int offsetSize,
bool isLarge)
{
int valueHeader =
- ((fieldIdSize - 1) & 0x03) |
- (((offsetSize - 1) & 0x03) << 2) |
+ ((offsetSize - 1) & 0x03) |
+ (((fieldIdSize - 1) & 0x03) << 2) |
((isLarge ? 1 : 0) << 4);
return MakeValueHeader(VariantBasicType.Object, valueHeader);
}
@@ -130,8 +130,8 @@ namespace Apache.Arrow.Scalars.Variant
public static void ParseObjectHeader(byte header, out int fieldIdSize,
out int offsetSize, out bool isLarge)
{
int valueHeader = GetValueHeader(header);
- fieldIdSize = (valueHeader & 0x03) + 1;
- offsetSize = ((valueHeader >> 2) & 0x03) + 1;
+ offsetSize = (valueHeader & 0x03) + 1;
+ fieldIdSize = ((valueHeader >> 2) & 0x03) + 1;
isLarge = ((valueHeader >> 4) & 0x01) != 0;
}
diff --git a/test/Apache.Arrow.Scalars.Tests/TestVectors.cs
b/test/Apache.Arrow.Scalars.Tests/TestVectors.cs
index 3c81622..70e7e14 100644
--- a/test/Apache.Arrow.Scalars.Tests/TestVectors.cs
+++ b/test/Apache.Arrow.Scalars.Tests/TestVectors.cs
@@ -325,6 +325,37 @@ namespace Apache.Arrow.Scalars.Tests
(byte)'B', (byte)'o', (byte)'b',
};
+ /// <summary>
+ /// The same object as <see cref="Object_Age30_Name_Bob"/> — {"age":
30, "name": "Bob"} —
+ /// but encoded with field_id_size=2 and offset_size=1.
+ ///
+ /// Every other object vector here uses field_id_size == offset_size,
where swapping the
+ /// two header fields is indistinguishable. This one does not, so it
detects a reader that
+ /// reads the two size fields from each other's bits: such a reader
sees field_id_size=1
+ /// and offset_size=2 and walks the id and offset lists at the wrong
widths.
+ ///
+ /// A wider-than-minimal field_id_size is legal; the spec requires
readers to honor the
+ /// width declared in the header.
+ /// </summary>
+ public static ReadOnlySpan<byte> Object_Age30_Name_Bob_WideFieldIds =>
new byte[]
+ {
+ 0x12, // header: basic_type=Object(2), fid_size=2,
off_size=1, is_large=false
+ // value_header = (fid_size-1) << 2 | (off_size-1) =
0b000100 = 4
+ // header byte = (4 << 2) | 2 = 0x12
+ 0x02, // num_fields = 2
+ 0x00, 0x00, // field_id[0] = 0 (=> "age"), 2 bytes little-endian
+ 0x01, 0x00, // field_id[1] = 1 (=> "name"), 2 bytes little-endian
+ 0x00, // offset[0] = 0
+ 0x02, // offset[1] = 2
+ 0x06, // end_offset = 6
+ // value 0: Int8 = 30
+ 0x0C, // primitive Int8 header
+ 0x1E, // 30
+ // value 1: short string "Bob"
+ 0x0D, // basic_type=ShortString(1), length=3 => (3 << 2) | 1
= 13
+ (byte)'B', (byte)'o', (byte)'b',
+ };
+
// =================================================================
// Array test vectors
// =================================================================
diff --git a/test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs
b/test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs
index f95bbf2..83589d9 100644
--- a/test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs
+++ b/test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs
@@ -97,6 +97,45 @@ namespace Apache.Arrow.Scalars.Tests
Assert.Equal(isLarge, parsedIsLarge);
}
+ // The round-trip test above cannot detect the field-id and offset
size bits being
+ // swapped, because MakeObjectHeader and ParseObjectHeader would share
the mistake. These
+ // two pin each direction to literal bytes taken from the spec's
layout instead:
+ //
+ // value_header bits 0-1 = field_offset_size - 1
+ // value_header bits 2-3 = field_id_size - 1
+ // value_header bit 4 = is_large
+ // header byte = (value_header << 2) | Object(2)
+
+ [Theory]
+ [InlineData(1, 1, false, 0x02)]
+ [InlineData(2, 1, false, 0x12)]
+ [InlineData(1, 2, false, 0x06)]
+ [InlineData(2, 3, false, 0x1A)]
+ [InlineData(4, 1, true, 0x72)]
+ [InlineData(1, 4, true, 0x4E)]
+ [InlineData(4, 4, true, 0x7E)]
+ public void MakeObjectHeaderUsesSpecBitLayout(int fieldIdSize, int
offsetSize, bool isLarge, int expected)
+ {
+ byte header = VariantEncodingHelper.MakeObjectHeader(fieldIdSize,
offsetSize, isLarge);
+ Assert.Equal(expected, (int)header);
+ }
+
+ [Theory]
+ [InlineData(0x02, 1, 1, false)]
+ [InlineData(0x12, 2, 1, false)]
+ [InlineData(0x06, 1, 2, false)]
+ [InlineData(0x1A, 2, 3, false)]
+ [InlineData(0x72, 4, 1, true)]
+ [InlineData(0x4E, 1, 4, true)]
+ [InlineData(0x7E, 4, 4, true)]
+ public void ParseObjectHeaderUsesSpecBitLayout(int header, int
expectedFieldIdSize, int expectedOffsetSize, bool expectedIsLarge)
+ {
+ VariantEncodingHelper.ParseObjectHeader((byte)header, out int
fieldIdSize, out int offsetSize, out bool isLarge);
+ Assert.Equal(expectedFieldIdSize, fieldIdSize);
+ Assert.Equal(expectedOffsetSize, offsetSize);
+ Assert.Equal(expectedIsLarge, isLarge);
+ }
+
// ---------------------------------------------------------------
// Array headers
// ---------------------------------------------------------------
diff --git a/test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs
b/test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs
index 10d72ef..37c8202 100644
--- a/test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs
+++ b/test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs
@@ -150,6 +150,53 @@ namespace Apache.Arrow.Scalars.Tests
Assert.False(obj.TryGetField("email", out VariantReader _));
}
+ // ---------------------------------------------------------------
+ // Asymmetric header sizes
+ // ---------------------------------------------------------------
+ //
+ // Objects whose field_id_size and offset_size differ are the only
ones that can detect
+ // the two header size fields being read from each other's bits. Every
vector above uses
+ // field_id_size == offset_size, where the two layouts coincide.
+
+ [Fact]
+ public void WideFieldIds_FieldNames()
+ {
+ VariantObjectReader obj = new VariantObjectReader(
+ TestVectors.SortedMetadata_Age_Name,
TestVectors.Object_Age30_Name_Bob_WideFieldIds);
+
+ Assert.Equal(2, obj.FieldCount);
+ Assert.Equal("age", obj.GetFieldName(0));
+ Assert.Equal("name", obj.GetFieldName(1));
+ }
+
+ [Fact]
+ public void WideFieldIds_FieldValues()
+ {
+ VariantObjectReader obj = new VariantObjectReader(
+ TestVectors.SortedMetadata_Age_Name,
TestVectors.Object_Age30_Name_Bob_WideFieldIds);
+
+ VariantReader ageValue = obj.GetFieldValue(0);
+ Assert.Equal(VariantPrimitiveType.Int8, ageValue.PrimitiveType);
+ Assert.Equal(30, ageValue.GetInt8());
+
+ VariantReader nameValue = obj.GetFieldValue(1);
+ Assert.True(nameValue.IsString);
+ Assert.Equal("Bob", nameValue.GetString());
+ }
+
+ [Fact]
+ public void WideFieldIds_TryGetField_Both()
+ {
+ VariantObjectReader obj = new VariantObjectReader(
+ TestVectors.SortedMetadata_Age_Name,
TestVectors.Object_Age30_Name_Bob_WideFieldIds);
+
+ Assert.True(obj.TryGetField("age", out VariantReader ageValue));
+ Assert.Equal(30, ageValue.GetInt8());
+
+ Assert.True(obj.TryGetField("name", out VariantReader nameValue));
+ Assert.Equal("Bob", nameValue.GetString());
+ }
+
// ---------------------------------------------------------------
// Error cases
// ---------------------------------------------------------------
