Update metrics tests to use Iceberg generics #1070
Conversation
This is to support ORC metrics using the same tests. ORC doesn't support writing Avro records.
| optional(11, "uuidCol", UUIDType.get()), | ||
| required(12, "fixedCol", FixedType.ofLength(4)), | ||
| required(13, "binaryCol", BinaryType.get()) | ||
| required(11, "fixedCol", FixedType.ofLength(4)), |
There was a problem hiding this comment.
Sorry if I'm missing some context, but why is the uuidCol dropped?
There was a problem hiding this comment.
UUID columns aren't currently supported in the Parquet writers for Iceberg generics. Rather than adding that support here, I thought it was better to remove it. We can add it back later, but I don't want to block ORC metrics on it.
There was a problem hiding this comment.
SGTM. Thanks for the context.
| } | ||
|
|
||
| @Test | ||
| public void testMetricsForRepeatedValues() throws IOException { |
There was a problem hiding this comment.
is the test name misleading? Seems like we are testing for multiple records, not repeated values in maps and lists
There was a problem hiding this comment.
I think it sounds like we were testing repeated values in maps and lists.
There was a problem hiding this comment.
Since this is an existing test name, I'd rather not update it in this PR. We can open a separate one to fix it if you think it is necessary.
| import org.junit.Rule; | ||
| import org.junit.rules.TemporaryFolder; | ||
|
|
||
| /** |
There was a problem hiding this comment.
nit: seems like comment is not adding anything more
| optional(11, "uuidCol", UUIDType.get()), | ||
| required(12, "fixedCol", FixedType.ofLength(4)), | ||
| required(13, "binaryCol", BinaryType.get()) | ||
| required(11, "fixedCol", FixedType.ofLength(4)), |
There was a problem hiding this comment.
SGTM. Thanks for the context.
3 participants
This updates
TestMetricsto use Iceberg generic records for its test cases. This is to avoid creating an Avro writer for ORC just to make the tests work.Using Iceberg generics for this test required moving the generic record classes to core so they are available to
TestMetricsin core. Reader and writer classes for Avro are also moved to core.