Core: Fix data loss in partial variant shredding

[dirtysalt]

When using variantShreddingFunc to partially shred variant fields, unshredded fields were being lost during serialization. The bug was in ShreddedObject constructor where local variable shreddedFields shadowed the instance field this.shreddedFields, causing unshredded fields to be added to the local map instead of the instance field.

This resulted in the binary value field containing only metadata headers without actual field data, causing IndexOutOfBoundsException on read and permanent data loss.

Fix: Changed all references in the problematic code block to use this.shreddedFields explicitly, ensuring unshredded fields are properly preserved in the instance field and serialized correctly.Added test case testPartialShreddingWithShreddedObject that reproduces the exact scenario from issue #15086.

[nastra]

@huaxingao @aihuaxu could you guys please take a look?

[amogh-jahagirdar]

Thanks for fixing this @dirtysalt! Though admittedly I'm a bit confused why our linter didn't flag the hidden fields, maybe something to check separately. I think I'd prefer to rename the fields to something like currentShreddedFields, currentUnshreddedFields (something that better indicates that it's state). That makes the code a bit more clear imo.

[aihuaxu]

+1 on this. Let's use different names to make it cleaner. Thanks for fixing it.

[huaxingao]

+1 on renaming the field

[rdblue]

The reason why the name conflict was allowed is that this is the constructor so the check assumes that shreddedFields is used to initialize this.shreddedFields, which is true.

I agree that this should use a different name for the method argument.

[amogh-jahagirdar]

Either way of renaming the field or the method argument sounds good to me!

[rdblue]

It's fewer changes if you rename the argument. Just two lines.

[rdblue]

Isn't there a more direct way to test this in TestShreddedObject? I think it's fine to have an end-to-end test, but I'd like to see a more direct one that doesn't go through Parquet.

[rdblue]

I think an end-to-end test should live in TestVariantReaders rather than TestVariantWriters. The problem you're trying to recreate is in the read path when a partially shredded object is being reconstructed. I think that the tests should also be in the read path.

[rdblue]

I looked into this a bit more and ended up producing a test for TestShreddedObject that reproduces the problem. Here's my full diff of the test and the fix:

diff --git a/core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java b/core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java
index c1fba073cf..5d2bfc48fa 100644
--- a/core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java
+++ b/core/src/main/java/org/apache/iceberg/variants/ShreddedObject.java
@@ -153,12 +153,12 @@ public class ShreddedObject implements VariantObject {
     private SerializationState(
         VariantMetadata metadata,
         VariantObject unshredded,
-        Map<String, VariantValue> shreddedFields,
+        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(shreddedFields);
+      this.shreddedFields = Maps.newHashMap(shredded);
 
       int totalDataSize = 0;
       // get the unshredded field names and values as byte buffers
diff --git a/core/src/test/java/org/apache/iceberg/variants/TestShreddedObject.java b/core/src/test/java/org/apache/iceberg/variants/TestShreddedObject.java
index 1ebb433a54..0cb043d420 100644
--- a/core/src/test/java/org/apache/iceberg/variants/TestShreddedObject.java
+++ b/core/src/test/java/org/apache/iceberg/variants/TestShreddedObject.java
@@ -213,6 +213,28 @@ public class TestShreddedObject {
         .isEqualTo(DateTimeUtil.isoDateToDays("2024-10-12"));
   }
 
+  @Test
+  public void testPartiallyShreddedUnserializedObjectSerializationMinimalBuffer() {
+    ShreddedObject partial = createUnserializedObject(FIELDS);
+    VariantMetadata metadata = partial.metadata();
+
+    // replace field c with a new value
+    partial.put("c", Variants.ofIsoDate("2024-10-12"));
+    partial.remove("b");
+
+    VariantValue value = roundTripMinimalBuffer(partial, metadata);
+
+    assertThat(value).isInstanceOf(SerializedObject.class);
+    SerializedObject actual = (SerializedObject) value;
+
+    assertThat(actual.get("a")).isInstanceOf(VariantPrimitive.class);
+    assertThat(actual.get("a").asPrimitive().get()).isEqualTo(34);
+    assertThat(actual.get("c")).isInstanceOf(VariantPrimitive.class);
+    assertThat(actual.get("c").type()).isEqualTo(PhysicalType.DATE);
+    assertThat(actual.get("c").asPrimitive().get())
+        .isEqualTo(DateTimeUtil.isoDateToDays("2024-10-12"));
+  }
+
   @Test
   public void testPartiallyShreddedObjectSerializationLargeBuffer() {
     ShreddedObject partial = createUnshreddedObject(FIELDS);
@@ -370,6 +392,12 @@ public class TestShreddedObject {
     return Variants.value(metadata, slice);
   }
 
+  private static ShreddedObject createUnserializedObject(Map<String, VariantValue> fields) {
+    ByteBuffer metadataBuffer = VariantTestUtil.createMetadata(fields.keySet(), false);
+    VariantMetadata metadata = SerializedMetadata.from(metadataBuffer);
+    return new ShreddedObject(metadata, createShreddedObject(metadata, fields));
+  }
+
   /** Creates a ShreddedObject with fields in its shredded map, using the given metadata */
   private static ShreddedObject createShreddedObject(
       VariantMetadata metadata, Map<String, VariantValue> fields) {

Feel free to use that test case. It would also be nice to have a Parquet test like this one. If you end up keeping it, please use helper methods to create the Parquet schema (see ParquetVariantUtil).