rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786453831
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -56,12 +58,38 @@ private ParquetValueWriters() {}
public static <T> ParquetValueWriter<T> option(
Type type, int definitionLevel, ParquetValueWriter<T> writer) {
if (type.isRepetition(Type.Repetition.OPTIONAL)) {
- return new OptionWriter<>(definitionLevel, writer);
+ return new OptionWriter<>(definitionLevel, writer,
optionalLeafIds(type));
}
return writer;
}
+ /**
+ * Collects the ids of optional primitive leaves within a type. A null
written for an optional
+ * value is also a null for these fields, but not for required fields, whose
null count is left as
+ * it was before (per the spec, null counts are only tracked for optional
fields).
+ */
+ private static Set<Integer> optionalLeafIds(Type type) {
Review Comment:
As I noted below, I don't think that the behavior depends on whether the
leaf type is optional. All fields should be updated if there is an optional
parent that results in using the option writer.
A couple of other notes for future contributions though:
- This isn't the right place to analyze the leaf IDs. We write most type
structure logic for Iceberg types so that it is reusable.
- Methods should not be written to process a tree structure recursively. The
project uses visitors to standardize how types are visited so that we have
consistent logic throughout. It also isolates the logic to traverse a type from
the logic for what to do with each case.
--
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]
