vbhanuchander-lang commented on issue #17510:
URL: https://github.com/apache/iceberg/issues/17510#issuecomment-5274333880
I went to pick this up and, after tracing it, I think the premise no longer
holds — no
`VariantValue` implementation serializes in order to report its size, so the
capability the
TODO asks for already exists.
**`sizeInBytes()` is contractually the serialized size** and is computed
arithmetically at
every level:
- `SerializedValue` (the base for `SerializedPrimitive`,
`SerializedShortString`,
`SerializedArray`, `SerializedObject`) returns `buffer().remaining()`, and
its `writeTo`
copies that buffer verbatim and returns `remaining()`. So for a serialized
value the
reported size *is* exactly the number of bytes written when embedded in a
parent.
- `ValueArray.sizeInBytes()`
([ValueArray.java:51](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/variants/ValueArray.java#L51))
delegates to `SerializationState.size()`, which is pure arithmetic —
`1 + (isLarge ? 4 : 1) + (1 + numElements) * offsetSize + dataSize` —
where `dataSize` is
the recursive sum of `value.sizeInBytes()`. No bytes are produced.
- `ShreddedObject.sizeInBytes()`
([ShreddedObject.java:122](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java#L122))
does the same, and goes out of its way to avoid materializing:
`// for serialized objects, use existing buffers instead of materializing
values`
([ShreddedObject.java:174](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java#L174)).
- `PrimitiveWrapper.sizeInBytes()` is a `switch` returning constants.
On the specific concern in the description — that a `SerializedArray` passed
as an element
"may not reflect the size needed when embedded in a parent array" —
`SerializedValue.writeTo`
writes precisely `buffer().remaining()` bytes, which is what `sizeInBytes()`
returned. The
parent's offset list lives outside its data section, so an embedded value
costs its own size
and nothing more.
There is also no redundant work in the `sizeInBytes()` → `writeTo()`
sequence that
`createArray` performs: both go through the same lazily built, cached
`SerializationState`,
which is what the comment at
[ValueArray.java:71](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/variants/ValueArray.java#L71)
records — *"Common state for `size()` and `writeTo()`"*. The state is
constructed once and
reused.
So `createArray`/`createObject` summing `sizeInBytes()` is correct and
already
serialization-free for nested and non-buffer-backed elements alike. My read
is that the TODO
at
[VariantTestUtil.java:273](https://github.com/apache/iceberg/blob/main/api/src/test/java/org/apache/iceberg/variants/VariantTestUtil.java#L273)
predates `SerializationState` and is simply stale.
If a committer agrees, this can be closed and the only change worth making
is deleting that
one stale comment line — happy to open that trivial PR. If instead I have
missed a concrete
case where `sizeInBytes()` is wrong or forces serialization, I would
genuinely like to see it,
since I could not construct one.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
