amogh-jahagirdar commented on code in PR #17320:
URL: https://github.com/apache/iceberg/pull/17320#discussion_r3653166694
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -280,29 +280,91 @@ public ParquetValueReader<?> struct(
}
}
- int constantDefinitionLevel = type.getMaxDefinitionLevel(currentPath());
+ String[] structPath = currentPath();
+ int constantDefinitionLevel = type.getMaxDefinitionLevel(structPath);
List<Types.NestedField> expectedFields = expected.fields();
+
+ // When a nullable struct's projected fields are all constants (initial
defaults, metadata
+ // columns, or partition values) with no real column read from the file,
the struct has no
+ // per-row definition level to indicate whether an ancestor struct is
null. In that case,
+ // borrow the definition level from a real leaf column that exists under
the struct in the
+ // file so a null ancestor is not incorrectly materialized as a struct
of default values.
+ boolean hasRealFieldReader =
+ expectedFields.stream().anyMatch(field ->
readersById.containsKey(field.fieldId()));
+ ColumnDescriptor probe = null;
+ Integer probeHostId = null;
+ if (!hasRealFieldReader && constantDefinitionLevel > 0) {
+ probe = firstLeafUnder(structPath);
+ if (probe != null) {
+ probeHostId = probeHostFieldId(expectedFields);
+ }
+ }
+
List<ParquetValueReader<?>> reorderedFields =
Lists.newArrayListWithExpectedSize(expectedFields.size());
-
for (Types.NestedField field : expectedFields) {
int id = field.fieldId();
ParquetValueReader<?> reader =
ParquetValueReaders.replaceWithMetadataReader(
id, readersById.get(id), idToConstant,
constantDefinitionLevel);
- reorderedFields.add(defaultReader(field, reader,
constantDefinitionLevel));
+ boolean hostsProbe = probeHostId != null && id == probeHostId;
+ reorderedFields.add(
Review Comment:
I thought about this scenario some more and I don't think it's valid because
the only condition where the idToConstant map has something for a nested source
is identity partitions.
But in that case it would be impossible to have a non-null idToConstant and
a null value in the file, because that violates the partitioning requirement (a
null nested field means the row belongs to the null partition, not the "US"
partition). The opposite situation (null idToConstant and non-null partition
value materialized in the file) would also not be allowed. The metadata columns
like row ID etc. are all top level IDs, so they never show up nested inside a
struct so the fix isn't relevant for that.
Non-identity transforms like day/bucket/truncate don't come into play here
either, since constantsMap only puts a source id into idToConstant when the
transform is identity.
So the initial default case is really the only one that needs this behavior,
which is what the PR handles.
Let me know what
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetReaders.java:
##########
@@ -280,29 +280,91 @@ public ParquetValueReader<?> struct(
}
}
- int constantDefinitionLevel = type.getMaxDefinitionLevel(currentPath());
+ String[] structPath = currentPath();
+ int constantDefinitionLevel = type.getMaxDefinitionLevel(structPath);
List<Types.NestedField> expectedFields = expected.fields();
+
+ // When a nullable struct's projected fields are all constants (initial
defaults, metadata
+ // columns, or partition values) with no real column read from the file,
the struct has no
+ // per-row definition level to indicate whether an ancestor struct is
null. In that case,
+ // borrow the definition level from a real leaf column that exists under
the struct in the
+ // file so a null ancestor is not incorrectly materialized as a struct
of default values.
+ boolean hasRealFieldReader =
+ expectedFields.stream().anyMatch(field ->
readersById.containsKey(field.fieldId()));
+ ColumnDescriptor probe = null;
+ Integer probeHostId = null;
+ if (!hasRealFieldReader && constantDefinitionLevel > 0) {
+ probe = firstLeafUnder(structPath);
+ if (probe != null) {
+ probeHostId = probeHostFieldId(expectedFields);
+ }
+ }
+
List<ParquetValueReader<?>> reorderedFields =
Lists.newArrayListWithExpectedSize(expectedFields.size());
-
for (Types.NestedField field : expectedFields) {
int id = field.fieldId();
ParquetValueReader<?> reader =
ParquetValueReaders.replaceWithMetadataReader(
id, readersById.get(id), idToConstant,
constantDefinitionLevel);
- reorderedFields.add(defaultReader(field, reader,
constantDefinitionLevel));
+ boolean hostsProbe = probeHostId != null && id == probeHostId;
+ reorderedFields.add(
Review Comment:
I thought about this scenario some more and I don't think it's valid because
the only condition where the idToConstant map has something for a nested source
is identity partitions.
But in that case it would be impossible to have a non-null idToConstant and
a null value in the file, because that violates the partitioning requirement (a
null nested field means the row belongs to the null partition, not the "US"
partition). The opposite situation (null idToConstant and non-null partition
value materialized in the file) would also not be allowed. The metadata columns
like row ID etc. are all top level IDs, so they never show up nested inside a
struct so the fix isn't relevant for that.
Non-identity transforms like day/bucket/truncate don't come into play here
either, since constantsMap only puts a source id into idToConstant when the
transform is identity.
So the initial default case is really the only one that needs this behavior,
which is what the PR handles.
Let me know what you think @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]
