huaxingao commented on code in PR #17114:
URL: https://github.com/apache/iceberg/pull/17114#discussion_r3599449484
##########
core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java:
##########
@@ -211,60 +212,60 @@ private int size() {
}
private int writeTo(ByteBuffer buffer, int offset) {
- int fieldIdListOffset =
- offset + 1 /* header size */ + (isLarge ? 4 : 1) /* num elements
size */;
+ int numElementsSize = isLarge ? 4 : 1;
+ int fieldIdListOffset = offset + 1 /* header size */ + numElementsSize;
int offsetListOffset = fieldIdListOffset + (numElements * fieldIdSize);
int dataOffset = offsetListOffset + ((1 + numElements) * offsetSize);
byte header = VariantUtil.objectHeader(isLarge, fieldIdSize, offsetSize);
ByteBuffers.writeByte(buffer, header, offset);
- ByteBuffers.writeLittleEndianUnsigned(buffer, numElements, offset + 1,
isLarge ? 4 : 1);
-
- // neither iterable is closeable, so it is okay to use Iterable
- Iterable<String> fields =
- SortedMerge.of(
- () -> unshreddedFields.keySet().stream().sorted().iterator(),
- () -> shreddedFields.keySet().stream().sorted().iterator());
+ ByteBuffers.writeLittleEndianUnsigned(buffer, numElements, offset + 1,
numElementsSize);
int nextValueOffset = 0;
- int index = 0;
- for (String field : fields) {
- // write the field ID from the metadata dictionary
- int id = metadata.id(field);
- Preconditions.checkState(id >= 0, "Invalid metadata, missing: %s",
field);
+ for (int index = 0; index < numElements; index++) {
+ Entry entry = entries[index];
ByteBuffers.writeLittleEndianUnsigned(
- buffer, id, fieldIdListOffset + (index * fieldIdSize),
fieldIdSize);
- // write the data offset
+ buffer, entry.id, fieldIdListOffset + (index * fieldIdSize),
fieldIdSize);
ByteBuffers.writeLittleEndianUnsigned(
buffer, nextValueOffset, offsetListOffset + (index * offsetSize),
offsetSize);
- // copy or serialize the value into the data section
- int valueSize;
- VariantValue shreddedValue = shreddedFields.get(field);
- if (shreddedValue != null) {
- valueSize = shreddedValue.writeTo(buffer, dataOffset +
nextValueOffset);
+ if (entry.shredded != null) {
+ entry.shredded.writeTo(buffer, dataOffset + nextValueOffset);
Review Comment:
Since the offset now trusts the precomputed entry.size rather than the bytes
actually written, could we guard the writeTo() == sizeInBytes() invariant
inline? Capturing the return and asserting it would catch any future
VariantValue whose writeTo/sizeInBytes disagree, which would otherwise silently
corrupt the offset list:
```
if (entry.shredded != null) {
int written = entry.shredded.writeTo(buffer, dataOffset + nextValueOffset);
Preconditions.checkState(
written == entry.size,
"Wrote %s bytes for field id %s but expected %s (writeTo and
sizeInBytes disagree)",
written,
entry.id,
entry.size);
} else {
...
}
```
--
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]
