amogh-jahagirdar commented on code in PR #17320:
URL: https://github.com/apache/iceberg/pull/17320#discussion_r3944405210
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetSchemaUtil.java:
##########
@@ -129,12 +131,94 @@ public static Type fieldType(GroupType group, String
name) {
public static MessageType pruneColumns(MessageType fileSchema, Schema
expectedSchema) {
// column order must match the incoming type, so it doesn't matter that
the ids are unordered
- Set<Integer> selectedIds = TypeUtil.getProjectedIds(expectedSchema);
+ Set<Integer> selectedIds =
Sets.newHashSet(TypeUtil.getProjectedIds(expectedSchema));
+ // retain one real leaf under each struct that projects only constants
like default values,
+ // so its definition level still shows whether the struct is null
+ TypeWithSchemaVisitor.visit(
+ expectedSchema.asStruct(),
+ fileSchema,
+ new DefinitionLevelProbeSelector(fileSchema, selectedIds));
return (MessageType)
TypeWithSchemaVisitor.visit(
expectedSchema.asStruct(), fileSchema, new
PruneColumns(selectedIds));
}
+ /**
+ * Adds one leaf id under each projected struct whose fields are all
constants and would otherwise
+ * retain no file leaf. That leaf's definition level is what still shows
whether the struct is
+ * null.
+ */
+ private static class DefinitionLevelProbeSelector extends
TypeWithSchemaVisitor<Void> {
+ private final MessageType fileSchema;
+ private final Set<Integer> selectedIds;
+
+ private DefinitionLevelProbeSelector(MessageType fileSchema, Set<Integer>
selectedIds) {
+ this.fileSchema = fileSchema;
+ this.selectedIds = selectedIds;
+ }
+
+ @Override
+ public Void struct(Types.StructType expected, GroupType struct, List<Void>
fields) {
+ // nothing projected under this struct, so there is nothing to probe for
+ if (expected == null || expected.fields().isEmpty()) {
+ return null;
+ }
+
+ // a struct that can never be null needs no probe
+ if (fileSchema.getMaxDefinitionLevel(currentPath()) <= 0) {
+ return null;
+ }
+
+ List<ColumnDescriptor> leaves = leafColumns(fileSchema, currentPath());
+ // add a probe leaf only if no real leaf under the struct is already read
+ boolean readsRealLeaf = leaves.stream().anyMatch(leaf ->
selectedIds.contains(leafId(leaf)));
+ if (!readsRealLeaf && !leaves.isEmpty()) {
+ selectedIds.add(leafId(leaves.get(0)));
+ }
+
+ return null;
+ }
+
+ @Override
+ public Void variant(Types.VariantType expected, GroupType variantGroup,
Void result) {
+ return null;
+ }
+ }
+
+ /** Returns the leaf columns with ids under the given path. */
+ static List<ColumnDescriptor> leafColumns(MessageType fileSchema, String[]
path) {
+ List<ColumnDescriptor> columns = Lists.newArrayList();
+ for (ColumnDescriptor column : fileSchema.getColumns()) {
+ // a probe leaf is kept in the read set by id, so a leaf without one
cannot be a probe
+ if (column.getPrimitiveType().getId() == null) {
Review Comment:
Great catch, to address this I made the leaf probe selection only pick a
leaf whose repetition level matches the struct's own, so no LIST/MAP sits
between them, and shared that logic between schema pruning and reader
construction so they agree. Since probe candidates have specific requirements
that may not match the schema, we bias towards failing the read rather than
silently falling back to the old broken behavior, so I added Preconditions
checks for that (e.g. a struct whose only fields are lists/maps, no flat leaf
anywhere)."
This does mean that there maybe some schemas + files with initial default
values that may fail to read but this is better than returning incorrect
results. For example, a nullable struct whose only fields are tags:
list<string> and tags2: map<string,string> (no flat scalar anywhere), read with
a schema that only projects a new added field with an initial default: there's
no leaf left to check whether the struct itself is null, so we now fail that
read instead of incorrectly applying the default to rows where the struct is
actually null.
--
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]
