rayokota opened a new issue, #420:
URL: https://github.com/apache/arrow-dotnet/issues/420
### Describe the bug, including details regarding any error messages,
version, and platform.
### Describe the bug
`VariantEncodingHelper` writes and reads the Variant object value header with
`field_id_size_minus_one` and `field_offset_size_minus_one` in each other's
bit positions.
The Variant spec (`apache/parquet-format`, `VariantEncoding.md`) defines the
object
`value_header` — the 6 bits above the 2 basic-type bits — as:
```
5 4 3 2 1 0
+---+---+-------+-------+
value_header | R | | | |
+---+---+-------+-------+
^ ^ ^
| | +-- field_offset_size_minus_one
| +-- field_id_size_minus_one
+-- is_large
```
That is: bits 0-1 hold `field_offset_size_minus_one`, bits 2-3 hold
`field_id_size_minus_one`.
`src/Apache.Arrow.Scalars/Variant/VariantEncodingHelper.cs` has them
inverted, in both
directions:
```csharp
// MakeObjectHeader, lines 121-122
((fieldIdSize - 1) & 0x03) | // bits 0-1 — spec: field_offset_size
(((offsetSize - 1) & 0x03) << 2) | // bits 2-3 — spec: field_id_size
// ParseObjectHeader, lines 133-134
fieldIdSize = (valueHeader & 0x03) + 1;
offsetSize = ((valueHeader >> 2) & 0x03) + 1;
```
The explanatory comment at lines 107-108 documents the same swapped layout,
so the whole
block is internally consistent — it is not a typo in a single expression.
`is_large` (bit 4) is correct. `MakeArrayHeader`/`ParseArrayHeader` and the
metadata header
helpers were checked and match the spec. This affects the object header only.
### Impact
Reader and writer share the inverted convention, so arrow-dotnet round-trips
its own output
correctly. The bug is only observable across implementations, and only when
`fieldIdSize != offsetSize` — when the two are equal, swapping them is a
no-op.
Those sizes are computed independently in `VariantValueWriter.cs:693-695`:
`fieldIdSize` from
the maximum field ID, `offsetSize` from the encoded data length. 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).
Consequences:
- Objects written by arrow-dotnet with differing sizes are misparsed by any
spec-compliant
reader (parquet-java, parquet-mr, Spark, arrow-rs).
- Objects written by those implementations are misparsed by arrow-dotnet.
- The failure is silent: field IDs and offsets are read at the wrong widths,
so it surfaces as
garbage field values or out-of-range offsets rather than a clean error.
### Why the tests do not catch it
`test/Apache.Arrow.Scalars.Tests/VariantEncodingHelperTests.cs:89-98` tests
`MakeObjectHeader` → `ParseObjectHeader` as a round-trip. Because both sides
share the
inverted convention, this test cannot detect a swap; it passes even on its
asymmetric
`InlineData(2, 3, false)` case.
Every hand-written object vector in `TestVectors.cs` (lines 281, 295, 313)
uses header byte
`0x02`, i.e. `field_id_size=1, offset_size=1` — precisely the degenerate
case where the swap
is invisible.
### Expected behavior
`MakeObjectHeader(fieldIdSize: 2, offsetSize: 1, isLarge: false)` should
return `0x12`:
field_id_size_minus_one = 1 -> value_header bits 2-3
offset_size_minus_one = 0 -> value_header bits 0-1
value_header = 0b000100 = 0x04
header byte = (0x04 << 2) | Object(2) = 0x12
It currently returns `0x06`. `ParseObjectHeader(0x12)` should yield
`fieldIdSize=2, offsetSize=1`; it currently yields `fieldIdSize=1,
offsetSize=2`.
--
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]
