aihuaxu commented on code in PR #13219:
URL: https://github.com/apache/iceberg/pull/13219#discussion_r2127083589
##########
parquet/src/main/java/org/apache/iceberg/parquet/TripleIterator.java:
##########
@@ -21,7 +21,7 @@
import java.util.Iterator;
import org.apache.parquet.io.api.Binary;
-interface TripleIterator<T> extends Iterator<T> {
+public interface TripleIterator<T> extends Iterator<T> {
Review Comment:
I'm following the writer path which exposes TripleWriter and we need
TripleIterator to be public to be accessed in VariantReader.
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetReaders.java:
##########
@@ -497,6 +514,48 @@ protected MapData buildMap(ReusableMapData map) {
}
}
+ /** Variant reader to convert from Variant to Spark VariantVal */
+ private static class VariantReader implements ParquetValueReader<VariantVal>
{
Review Comment:
I made the change to extend `DelegatingValueReader`. But we need to change
`DelegatingValueReader` to public to be used here. Then with such change, It
has the similar warning in DelegatingValueReader that TripleIterator is private
but to be exposed.
##########
parquet/src/main/java/org/apache/iceberg/parquet/TripleIterator.java:
##########
@@ -21,7 +21,7 @@
import java.util.Iterator;
import org.apache.parquet.io.api.Binary;
-interface TripleIterator<T> extends Iterator<T> {
+public interface TripleIterator<T> extends Iterator<T> {
Review Comment:
I just came back from leave and let me check out that. Thanks for the
suggestion.
##########
core/src/test/java/org/apache/iceberg/RandomVariants.java:
##########
@@ -52,12 +52,13 @@ public static Variant randomVariant(Random random) {
metadata = VariantMetadata.empty();
}
- return Variant.of(metadata, randomVariant(random, metadata,
randomType(random)));
+ return Variant.of(metadata, randomVariant(random, metadata,
randomType(random, excludedTypes)));
}
- private static PhysicalType randomType(Random random) {
- PhysicalType[] types = PhysicalType.values();
- return types[random.nextInt(types.length)];
+ private static PhysicalType randomType(Random random, PhysicalType...
excludedTypes) {
+ List<PhysicalType> types = Lists.newArrayList(PhysicalType.values());
+ types.removeAll(List.of(excludedTypes));
Review Comment:
Seems we don't need that from our test anymore so I just removed this.
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkParquetWriter.java:
##########
@@ -93,7 +94,8 @@ public class TestSparkParquetWriter {
optional(22, "jumpy", Types.DoubleType.get()),
required(23, "koala", Types.UUIDType.get()),
required(24, "couch rope", Types.IntegerType.get())))),
- optional(2, "slide", Types.StringType.get()));
+ optional(2, "slide", Types.StringType.get()),
+ required(25, "v", Types.VariantType.get()));
Review Comment:
Just chose flush. :)
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java:
##########
@@ -108,8 +112,8 @@ protected boolean supportsRowLineage() {
required(114, "dec_9_0", Types.DecimalType.of(9, 0)), // int encoded
required(115, "dec_11_2", Types.DecimalType.of(11, 2)), // long
encoded
required(116, "dec_20_5", Types.DecimalType.of(20, 5)), // requires
padding
- required(117, "dec_38_10", Types.DecimalType.of(38, 10)) // Spark's
maximum precision
- );
+ required(117, "dec_38_10", Types.DecimalType.of(38, 10)), // Spark's
maximum precision
+ required(118, "variant", Types.VariantType.get()));
Review Comment:
Updated. time is not supported in Spark so I added to skip time.
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/data/GenericsHelpers.java:
##########
@@ -208,6 +214,16 @@ private static void assertEqualsSafe(Type type, Object
expected, Object actual)
mapAsJavaMapConverter((scala.collection.Map<String, ?>)
actual).asJava();
assertEqualsSafe(type.asNestedType().asMapType(), (Map<?, ?>)
expected, asMap);
break;
+ case VARIANT:
+ assertThat(expected).as("Should expect a
Record").isInstanceOf(Record.class);
Review Comment:
Updated. Should be 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: [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]
