Copilot commented on code in PR #422: URL: https://github.com/apache/arrow-dotnet/pull/422#discussion_r3838785355
########## test/Apache.Arrow.Scalars.Tests/VariantObjectHeaderSizeTests.cs: ########## @@ -0,0 +1,361 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Collections.Generic; +using Apache.Arrow.Scalars.Variant; +using Xunit; + +namespace Apache.Arrow.Scalars.Tests +{ + /// <summary> + /// End-to-end coverage of the object value header's <c>field_id_size</c> and + /// <c>offset_size</c> bits, over the matrix of widths the two can take. + /// </summary> + /// <remarks> + /// <para> + /// <see cref="VariantEncodingHelperTests"/> pins <c>MakeObjectHeader</c> and + /// <c>ParseObjectHeader</c> to literal header bytes, but only at the helper level: it says + /// nothing about which widths <see cref="VariantValueWriter"/> actually asks for, or whether + /// the body it emits is laid out at those widths. These tests build real objects — a padded + /// dictionary to drive the field IDs up, a padded value to drive the data length up — assert + /// the header byte that lands in the output, and then decode the object body by hand against + /// the spec rather than through the library. + /// </para> + /// <para> + /// The interesting cells are the asymmetric ones. When <c>field_id_size == offset_size</c> + /// the two 2-bit fields are interchangeable and a transposition is invisible. The writer + /// computes them independently — IDs from the metadata dictionary, offsets from the encoded + /// data length — so in practice they diverge routinely. + /// </para> + /// <para> + /// Per <c>apache/parquet-format</c> <c>VariantEncoding.md</c>, an object header byte is: + /// </para> + /// <code> + /// bits 0-1: basic_type = 2 (Object) + /// bits 2-3: field_offset_size_minus_one + /// bits 4-5: field_id_size_minus_one + /// bit 6: is_large + /// bit 7: unused + /// </code> + /// <para> + /// Width 4 is not reachable from a real object in a unit test: a 4-byte field ID needs a + /// metadata dictionary of more than 16,777,216 entries, and a 4-byte offset needs more than + /// 16 MiB of field data. Those cells stay covered at the helper level only. + /// </para> + /// </remarks> + public class VariantObjectHeaderSizeTests + { + // Smallest values that need 2- and 3-byte encoding, for both field IDs and offsets. + private const int TwoByteThreshold = 0x100; + private const int ThreeByteThreshold = 0x10000; + + // Padded value lengths that put the object's end offset in each width's band, with room + // to spare for the remaining fields. + private const int PadForTwoByteOffsets = 300; + private const int PadForThreeByteOffsets = 70000; + + private const int SmallFieldCount = 2; + + // is_large is set for more than 255 fields. + private const int LargeFieldCount = 300; + + // --------------------------------------------------------------- + // The matrix + // --------------------------------------------------------------- + // + // expected header = (field_id_size - 1) << 4 | (offset_size - 1) << 2 | Object(2) + + [Theory] + [InlineData(1, 1, 0x02)] + [InlineData(1, 2, 0x06)] + [InlineData(1, 3, 0x0A)] + [InlineData(2, 1, 0x12)] + [InlineData(2, 2, 0x16)] + [InlineData(2, 3, 0x1A)] + [InlineData(3, 1, 0x22)] + [InlineData(3, 2, 0x26)] + [InlineData(3, 3, 0x2A)] + public void ObjectHeaderUsesSpecBitLayout(int fieldIdSize, int offsetSize, int expectedHeader) => + AssertObjectHeader(fieldIdSize, offsetSize, SmallFieldCount, expectedHeader); + + // Objects with more than 255 fields also set is_large (bit 6), on top of the two size + // fields. Such an object always carries at least 2-byte IDs (its own IDs run past 255) + // and at least 2-byte offsets (300 values do not fit in 255 bytes). + + [Theory] + [InlineData(2, 2, 0x56)] + [InlineData(2, 3, 0x5A)] + [InlineData(3, 2, 0x66)] + [InlineData(3, 3, 0x6A)] + public void LargeObjectHeaderUsesSpecBitLayout(int fieldIdSize, int offsetSize, int expectedHeader) => + AssertObjectHeader(fieldIdSize, offsetSize, LargeFieldCount, expectedHeader); + + private static void AssertObjectHeader(int fieldIdSize, int offsetSize, int fieldCount, int expectedHeader) + { + EncodedObject encoded = BuildObject(fieldIdSize, offsetSize, fieldCount); + + Assert.Equal(expectedHeader, (int)encoded.Value[0]); + + // Decoded here rather than through VariantEncodingHelper: the point is to check the + // writer against the spec, not against the reader that shares its convention. + ObjectLayout layout = DecodeObjectPerSpec(encoded.Value); + Assert.Equal(fieldIdSize, layout.FieldIdSize); + Assert.Equal(offsetSize, layout.OffsetSize); + Assert.Equal(fieldCount > 255, layout.IsLarge); + Assert.Equal(fieldCount, layout.FieldCount); + + // The IDs and offsets must make sense when read at the declared widths. Had the + // writer laid the body out at the other width, these lists would be garbage even + // though the header byte above is the one the spec asks for. + for (int i = 0; i < fieldCount; i++) + { + Assert.Equal(encoded.FirstFieldId + i, layout.FieldIds[i]); + } + + Assert.Equal(0, layout.Offsets[0]); + for (int i = 0; i < fieldCount; i++) + { + Assert.True(layout.Offsets[i + 1] > layout.Offsets[i], "offsets are not increasing at index " + i); + } + Assert.Equal(encoded.Value.Length - layout.DataStart, layout.Offsets[fieldCount]); Review Comment: The test asserts offsets are strictly increasing, but the variant object spec (and the library’s own VariantObjectReader remarks) allow non-monotonic offsets (field IDs must be sorted by name, but values may be stored in a different physical order). This makes the test more restrictive than “decode per spec” and could fail if the writer changes to a different (still spec-compliant) physical layout. Consider relaxing this to only validate that offsets are within the field-data region and that the last offset matches the total data length. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
