huaxingao commented on code in PR #17114:
URL: https://github.com/apache/iceberg/pull/17114#discussion_r3599459679
##########
core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java:
##########
@@ -157,48 +153,53 @@ private SerializationState(
VariantObject unshredded,
Map<String, VariantValue> shredded,
Set<String> removedFields) {
- this.metadata = metadata;
- // field ID size is the size needed to store the largest field ID in the
data
this.fieldIdSize = VariantUtil.sizeOf(metadata.dictionarySize());
- this.shreddedFields = Maps.newHashMap(shredded);
+ Map<String, Entry> sorted = Maps.newTreeMap();
int totalDataSize = 0;
- // get the unshredded field names and values as byte buffers
- ImmutableMap.Builder<String, ByteBuffer> unshreddedBuilder =
ImmutableMap.builder();
+
+ for (Map.Entry<String, VariantValue> field : shredded.entrySet()) {
+ String name = field.getKey();
+ int id = metadata.id(name);
+ Preconditions.checkState(id >= 0, "Invalid metadata, missing: %s",
name);
+ VariantValue value = field.getValue();
+ int valueSize = value.sizeInBytes();
+ sorted.put(name, Entry.ofShredded(id, value, valueSize));
+ totalDataSize += valueSize;
+ }
+
if (unshredded instanceof SerializedObject) {
- // for serialized objects, use existing buffers instead of
materializing values
Review Comment:
Nit: The refactor dropped a few comments explaining non-obvious intent, e.g.
this one about reusing existing buffers for serialized objects instead of
materializing values, and the isLarge/offsetSize sizing rationale. Could you
restore those?
--
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]
