amogh-jahagirdar commented on code in PR #17320:
URL: https://github.com/apache/iceberg/pull/17320#discussion_r3925803644
##########
data/src/test/java/org/apache/iceberg/data/parquet/TestGenericData.java:
##########
@@ -182,4 +184,154 @@ public void testTwoLevelList() throws IOException {
assertThat(Lists.newArrayList(reader)).hasSize(1);
}
}
+
+ @Test
Review Comment:
i'm a little skeptical this test is needed now, feels a bit duplicative
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -231,6 +251,105 @@ public static ParquetValueReader<?>
replaceWithMetadataReader(
return reader;
}
+ /**
+ * Builds readers for a struct's expected fields, in field order. A field
present in the file uses
+ * its column reader; a field missing from the file uses a metadata or
partition constant, or its
+ * initial default. When no expected field reads a file column, one default
reader is given a
+ * probe column so its definition level tracks the struct's null-ness.
+ */
+ public static List<ParquetValueReader<?>> structFieldReaders(
+ MessageType fileSchema,
+ String[] structPath,
+ List<Types.NestedField> expectedFields,
+ Map<Integer, ParquetValueReader<?>> readersById,
+ Map<Integer, ?> idToConstant,
+ BiFunction<org.apache.iceberg.types.Type, Object, Object>
convertConstant) {
+ int constantDefinitionLevel = fileSchema.getMaxDefinitionLevel(structPath);
+ ColumnDescriptor probe =
+ definitionLevelProbe(
+ fileSchema, structPath, constantDefinitionLevel, expectedFields,
readersById);
+ Integer probeHostId = probe == null ? null :
firstInitialDefaultFieldId(expectedFields);
+
+ List<ParquetValueReader<?>> readers =
Lists.newArrayListWithExpectedSize(expectedFields.size());
+ for (Types.NestedField field : expectedFields) {
+ int id = field.fieldId();
+ ParquetValueReader<?> reader =
+ replaceWithMetadataReader(id, readersById.get(id), idToConstant,
constantDefinitionLevel);
+ ColumnDescriptor fieldProbe = probeHostId != null && id == probeHostId ?
probe : null;
+ readers.add(
+ defaultReader(field, reader, constantDefinitionLevel, fieldProbe,
convertConstant));
+ }
+
+ return readers;
+ }
+
+ private static ParquetValueReader<?> defaultReader(
+ Types.NestedField field,
+ ParquetValueReader<?> reader,
+ int constantDefinitionLevel,
+ ColumnDescriptor probe,
+ BiFunction<org.apache.iceberg.types.Type, Object, Object>
convertConstant) {
+ if (reader != null) {
+ return reader;
+ } else if (field.initialDefault() != null) {
+ Object value = convertConstant.apply(field.type(),
field.initialDefault());
+ return probe != null ? constant(value, probe) : constant(value,
constantDefinitionLevel);
+ } else if (field.isOptional()) {
+ return nulls();
+ }
+
+ throw new IllegalArgumentException(String.format("Missing required field:
%s", field.name()));
+ }
+
+ /**
+ * Returns the first leaf column under the struct, or null if an expected
field already reads a
+ * file column or the struct has no leaf columns.
+ */
+ private static ColumnDescriptor definitionLevelProbe(
Review Comment:
Done, but what I did was make firstNonNullColumn choose a real materialized
column instead of the constant reader definition level. Though to be clear,
this is purely just to be defensive and robust to any change in how constant
readers may work in the future. the constant reader scenario isn't quite
possible today because it's only used for identity partitions, and that's not
possible to both be null AND have some other value as it goes against the
partitioning definition.
But I agree though it is best to write this in a manner which doesn't make
any assumptions on how it's being used today.
--
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]
