From 2204a2d97f8c29010ac5ec276d6f4c613d3fd38b Mon Sep 17 00:00:00 2001 From: Robert Yokota Date: Fri, 21 Aug 2026 21:58:13 -0700 Subject: [PATCH] fix: encoding of Variant object header field-id and offset sizes --- .../Variant/VariantEncodingHelper.cs | 12 ++--- .../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 6975bfbb..5e87c21d 100644 --- a/src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs +++ b/src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs @@ -104,8 +104,8 @@ public static byte MakeShortStringHeader(int length) => // 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 @@ public static byte MakeShortStringHeader(int length) => 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 @@ public static byte MakeObjectHeader(int fieldIdSize, int offsetSize, bool isLarg 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 3c81622d..70e7e145 100644 --- a/test/Apache.Arrow.Scalars.Tests/TestVectors.cs +++ b/test/Apache.Arrow.Scalars.Tests/TestVectors.cs @@ -325,6 +325,37 @@ internal static class TestVectors (byte)'B', (byte)'o', (byte)'b', }; + /// + /// The same object as — {"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. + /// + public static ReadOnlySpan 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 f95bbf26..83589d9c 100644 --- a/test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs +++ b/test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs @@ -97,6 +97,45 @@ public void MakeAndParseObjectHeader(int fieldIdSize, int offsetSize, bool isLar 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 10d72ef9..37c82025 100644 --- a/test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs +++ b/test/Apache.Arrow.Scalars.Tests/VariantReaderObjectTests.cs @@ -150,6 +150,53 @@ public void TwoFields_TryGetField_Both() 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 // ---------------------------------------------------------------