rdblue commented on code in PR #13219: URL: https://github.com/apache/iceberg/pull/13219#discussion_r2229605259
########## spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkVariants.java: ########## @@ -0,0 +1,238 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.spark.data; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.UUID; +import org.apache.iceberg.spark.TestBase; +import org.apache.iceberg.types.Types; +import org.apache.iceberg.variants.ShreddedObject; +import org.apache.iceberg.variants.ValueArray; +import org.apache.iceberg.variants.Variant; +import org.apache.iceberg.variants.VariantMetadata; +import org.apache.iceberg.variants.VariantValue; +import org.apache.iceberg.variants.Variants; +import org.apache.spark.sql.types.VariantType$; +import org.apache.spark.unsafe.types.VariantVal; +import org.junit.jupiter.api.Test; + +public class TestSparkVariants extends TestBase { + + @Test + public void testIcebergVariantTypeToSparkVariantType() { + // Test that Iceberg's VariantType converts to Spark's VariantType + Types.VariantType icebergVariantType = Types.VariantType.get(); + + // This would be done by TypeToSparkType converter + // For now, we just verify the types exist and can be referenced + assertThat(icebergVariantType).isNotNull(); + assertThat(VariantType$.MODULE$).isNotNull(); + } + + @Test + public void testSparkVariantTypeToIcebergVariantType() { + // Test that Spark's VariantType converts to Iceberg's VariantType + org.apache.spark.sql.types.DataType sparkVariantType = VariantType$.MODULE$; + + // This would be done by SparkTypeToType converter + // For now, we just verify the types exist and can be referenced + assertThat(sparkVariantType).isNotNull(); + assertThat(Types.VariantType.get()).isNotNull(); + } + + @Test + public void testVariantValueSerialization() { + // Test that Iceberg Variant can be serialized to bytes that are compatible with Spark's + // VariantVal + VariantMetadata metadata = Variants.emptyMetadata(); + VariantValue stringValue = Variants.of("test"); + Variant variant = Variant.of(metadata, stringValue); + + // Serialize the variant + ByteBuffer metadataBuffer = + ByteBuffer.allocate(metadata.sizeInBytes()).order(ByteOrder.LITTLE_ENDIAN); + metadata.writeTo(metadataBuffer, 0); + + ByteBuffer valueBuffer = + ByteBuffer.allocate(stringValue.sizeInBytes()).order(ByteOrder.LITTLE_ENDIAN); + stringValue.writeTo(valueBuffer, 0); + + // Verify serialization worked - the writeTo method doesn't change buffer position + // Instead, verify that the data was written correctly by checking the buffer contents + assertThat(metadataBuffer.array().length).isEqualTo(metadata.sizeInBytes()); + assertThat(valueBuffer.array().length).isEqualTo(stringValue.sizeInBytes()); + + // Test that we can create a VariantVal from the serialized bytes + byte[] metadataBytes = metadataBuffer.array(); + byte[] valueBytes = valueBuffer.array(); + + // This tests the integration point - Spark VariantVal should be able to use these bytes + VariantVal sparkVariant = new VariantVal(valueBytes, metadataBytes); + assertThat(sparkVariant).isNotNull(); + } + + @Test + public void testVariantPrimitiveRoundTrip() { + // Test various primitive types + testPrimitiveVariant(Variants.ofNull(), null); + testPrimitiveVariant(Variants.of(true), true); + testPrimitiveVariant(Variants.of(false), false); + testPrimitiveVariant(Variants.of((byte) 42), (byte) 42); + testPrimitiveVariant(Variants.of((short) 1000), (short) 1000); + testPrimitiveVariant(Variants.of(123456), 123456); + testPrimitiveVariant(Variants.of(123456789L), 123456789L); + testPrimitiveVariant(Variants.of(3.14f), 3.14f); + testPrimitiveVariant(Variants.of(2.71828), 2.71828); + testPrimitiveVariant(Variants.of("hello world"), "hello world"); + } + + @Test + public void testVariantArrayRoundTrip() { + VariantMetadata metadata = Variants.emptyMetadata(); + ValueArray array = Variants.array(); + array.add(Variants.of("hello")); + array.add(Variants.of(42)); + array.add(Variants.ofNull()); + + testVariantRoundTrip(metadata, array); + } + + @Test + public void testVariantObjectRoundTrip() { + VariantMetadata metadata = Variants.metadata("name", "age", "active"); + ShreddedObject object = Variants.object(metadata); + object.put("name", Variants.of("John Doe")); + object.put("age", Variants.of(30)); + object.put("active", Variants.of(true)); + + testVariantRoundTrip(metadata, object); + } + + @Test + public void testVariantNestedStructures() { + VariantMetadata metadata = Variants.metadata("user", "scores", "address", "city", "state"); + + // Create nested object: address + ShreddedObject address = Variants.object(metadata); + address.put("city", Variants.of("Anytown")); + address.put("state", Variants.of("CA")); + + // Create array of scores + ValueArray scores = Variants.array(); + scores.add(Variants.of(95)); + scores.add(Variants.of(87)); + scores.add(Variants.of(92)); + + // Create main object + ShreddedObject mainObject = Variants.object(metadata); + mainObject.put("user", Variants.of("Jane")); + mainObject.put("scores", scores); + mainObject.put("address", address); + + testVariantRoundTrip(metadata, mainObject); + } + + @Test + public void testVariantDateTimeTypes() { + // Test date/time variants + // Calculate the correct day offset for 2024-01-15 + testPrimitiveVariant(Variants.ofIsoDate("2024-01-15"), 19737); // days since epoch (corrected) + testPrimitiveVariant(Variants.ofIsoTimestamptz("2024-01-15T10:30:00Z"), 1705314600000000L); + testPrimitiveVariant(Variants.ofIsoTimestampntz("2024-01-15T10:30:00"), 1705314600000000L); + testPrimitiveVariant(Variants.ofIsoTime("10:30:00"), 37800000000L); + } + + @Test + public void testVariantBinaryAndUUID() { + // Test binary and UUID variants + byte[] binaryData = {1, 2, 3, 4, 5}; + testPrimitiveVariant(Variants.of(ByteBuffer.wrap(binaryData)), ByteBuffer.wrap(binaryData)); + + UUID uuid = UUID.randomUUID(); + testPrimitiveVariant(Variants.ofUUID(uuid), uuid); + } + + @Test + public void testUnsupportedOperations() { + // Test that basic operations work + // This tests the current state where Spark integration is not fully implemented + + // These tests document the current behavior and should be updated + // when full Spark integration is implemented + + // This should not throw - just verify the classes exist and basic operations work + VariantVal sparkVariant = new VariantVal(new byte[] {0}, new byte[] {1, 0}); + assertThat(sparkVariant.toString()).isNotNull(); // Should not throw + assertThat(sparkVariant.getValue()).isNotNull(); + assertThat(sparkVariant.getMetadata()).isNotNull(); + } + + private void testPrimitiveVariant(VariantValue value, Object expectedValue) { + VariantMetadata metadata = Variants.emptyMetadata(); + testVariantRoundTrip(metadata, value); + + if (expectedValue != null) { + assertThat(value.asPrimitive().get()).isEqualTo(expectedValue); + } else { + assertThat(value.asPrimitive().get()).isNull(); + } + } + + private void testVariantRoundTrip(VariantMetadata metadata, VariantValue value) { Review Comment: I noted this above, but `VariantVal` is a holder and doesn't really validate or work with the bytes at all: ```java public class VariantVal implements Serializable { protected final byte[] value; protected final byte[] metadata; public VariantVal(byte[] value, byte[] metadata) { this.value = value; this.metadata = metadata; } public byte[] getValue() { return this.value; } public byte[] getMetadata() { return this.metadata; } ... } ``` The serialization here is performing the same operations as the serialization tests in the variants package, and those check things like serialization into the middle of a buffer and back. It looks like tests against Spark's implementation could use `org.apache.spark.types.variant.Variant` instead. You could write a helper to assert that a Spark `Variant` created from the `metadata` and `value` buffers is equivalent to the Iceberg source variant. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
