rdblue commented on code in PR #12512:
URL: https://github.com/apache/iceberg/pull/12512#discussion_r2059369475
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantReaders.java:
##########
@@ -900,6 +1316,102 @@ private static GenericRecord record(GroupType type,
Map<String, Object> fields)
return record;
}
+ private static List<GenericRecord> elements(GroupType elementType,
List<VariantValue> elements) {
Review Comment:
I spent a good amount of time trying to make this work, but I think that
these helper methods to convert variants to Avro should not be used and can be
removed.'
The problem is that the recursive conversion to Avro makes the tests
confusing and it doesn't seem to verify that the values are actually shredded
because it relies on `shreddedType(value).equals(shreddedType)` to determine
whether to create a record with `value` or `typed_value`. The rest of the tests
in the suite are explicit about which field is set so we know that if the write
succeeded, we are actually testing with a shredded or unshredded field.
Removing this also makes the tests more readable and clean. Let's use
`testSimpleArray` as an example:
With these helpers, the code is much less clear (is this going to be
shredded?):
```java
List<GenericRecord> arr =
elements(elementType, List.of(Variants.of("comedy"),
Variants.of("drama")));
```
Without the helpers you can see exactly what is happening:
```java
List<GenericRecord> arr =
List.of(
record(elementType, Map.of("typed_value", "comedy")),
record(elementType, Map.of("typed_value", "drama")));
```
--
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]
