amogh-jahagirdar commented on code in PR #17320:
URL: https://github.com/apache/iceberg/pull/17320#discussion_r3944379814
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java:
##########
@@ -231,6 +251,87 @@ 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();
Review Comment:
Hm, this maybe a separate issue. The test doesn't set up an initial default
though the failure in the test does indicate some other issue that's unrelated
to default values.
This issue looks like a pre-existing gap in how a struct's own presence gets
tracked when none of its projected fields end up reading anything real from the
file, whether that's because a field has no default or just because it's a
plain missing optional field doesn't matter, the struct reader has nothing left
to check against the file either way.
e.g.
```
Example. File has two rows:
row 1: id=1, nested={inner: "a"} <- nested is present
row 2: id=2, nested=null <- nested is null
You read with a projection where nested only asks for a field added that
doesn't exist in the file, and has no default.
Expected:
row 1: nested = {added: null} (nested was present, just has nothing for
"added")
row 2: nested = null
Actual (the bug):
row 1: nested = null <- WRONG, should be {added: null}
row 2: nested = null (this one happens to be right)
```
I think it's worth its own issue rather than folding into this one since
it's independent of default values and seems like some long standing
pre-existing behavior. Wdyt @pvary ?
--
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]
