rdblue commented on code in PR #12836:
URL: https://github.com/apache/iceberg/pull/12836#discussion_r2051039187
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -237,55 +236,37 @@ public ParquetValueReader<?> struct(
int fieldD = type.getMaxDefinitionLevel(path(fieldType.getName())) -
1;
int id = fieldType.getId().intValue();
readersById.put(id, ParquetValueReaders.option(fieldType, fieldD,
fieldReader));
- typesById.put(id, fieldType);
- if (idToConstant.containsKey(id)) {
- maxDefinitionLevelsById.put(id, fieldD);
- }
}
}
- List<Types.NestedField> expectedFields =
- expected != null ? expected.fields() : ImmutableList.of();
+ int constantDefinitionLevel = type.getMaxDefinitionLevel(currentPath());
+ List<Types.NestedField> expectedFields = expected.fields();
List<ParquetValueReader<?>> reorderedFields =
Lists.newArrayListWithExpectedSize(expectedFields.size());
- List<Type> types =
Lists.newArrayListWithExpectedSize(expectedFields.size());
- // Defaulting to parent max definition level
- int defaultMaxDefinitionLevel =
type.getMaxDefinitionLevel(currentPath());
+
for (Types.NestedField field : expectedFields) {
int id = field.fieldId();
- ParquetValueReader<?> reader = readersById.get(id);
- if (idToConstant.containsKey(id)) {
- // containsKey is used because the constant may be null
- int fieldMaxDefinitionLevel =
- maxDefinitionLevelsById.getOrDefault(id,
defaultMaxDefinitionLevel);
- reorderedFields.add(
- ParquetValueReaders.constant(idToConstant.get(id),
fieldMaxDefinitionLevel));
- types.add(null);
- } else if (id == MetadataColumns.ROW_POSITION.fieldId()) {
- reorderedFields.add(ParquetValueReaders.position());
- types.add(null);
- } else if (id == MetadataColumns.IS_DELETED.fieldId()) {
- reorderedFields.add(ParquetValueReaders.constant(false));
- types.add(null);
- } else if (reader != null) {
- reorderedFields.add(reader);
- types.add(typesById.get(id));
- } else if (field.initialDefault() != null) {
- reorderedFields.add(
- ParquetValueReaders.constant(
- convertConstant(field.type(), field.initialDefault()),
- maxDefinitionLevelsById.getOrDefault(id,
defaultMaxDefinitionLevel)));
- types.add(typesById.get(id));
- } else if (field.isOptional()) {
- reorderedFields.add(ParquetValueReaders.nulls());
- types.add(null);
- } else {
- throw new IllegalArgumentException(
- String.format("Missing required field: %s", field.name()));
- }
+ ParquetValueReader<?> reader =
+ ParquetValueReaders.replaceWithMetadataReader(
+ id, readersById.get(id), idToConstant,
constantDefinitionLevel);
+ reorderedFields.add(defaultReader(field, reader,
constantDefinitionLevel));
Review Comment:
The changes here were needed to reduce the complexity of the `struct` method
that was failing checkstyle. I refactored into 2 components:
* `ParquetValueReaders.replaceWithMetadataReader` handles metadata columns
and constants
* `defaultReader` handles defaults based on the schema; this requires
`convertConstant` so it is not shared in `ParquetValueReaders`
I also simplified the constant handling that was introduced in #4627. That
PR introduced a map with the max definition level for columns that are replaced
with constants and defaults the definition level to the parent struct's value
when there is no underlying column (for instance, `_pos`). This was
over-complicated because the fields don't need a column-specific DL. If the
parent struct is non-null then the constant should be added to it, so the DL
can always be the parent's DL.
--
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
