rdblue commented on code in PR #17560:
URL: https://github.com/apache/iceberg/pull/17560#discussion_r3786478826
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetValueWriters.java:
##########
@@ -60,4 +68,144 @@ void geospatialValueSizeMetricsExcludeNulls() {
assertThat(metrics.nullValueCount()).isEqualTo(1);
assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31);
}
+
+ @Test
+ void nullStructCountsNullsForOptionalNestedFields() {
+ // a null struct is also null for the optional fields it contains, but
those columns are written
+ // by the struct's writer and never see the value, so the struct must
count the nulls for them
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), optional(3, "f",
Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // both fields have one non-null value and two nulls from the null structs
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(3);
+ }
+
+ @Test
+ void nullStructDoesNotCountNullsForRequiredNestedFields() {
+ // null counts are only tracked for optional fields, so a required leaf
keeps its prior
+ // behavior: the null struct's nulls are not added to it
+ Types.StructType struct =
+ Types.StructType.of(
+ optional(2, "d", Types.DoubleType.get()), required(3, "f",
Types.FloatType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record inner = GenericRecord.create(struct);
+ inner.set(0, 2.0D);
+ inner.set(1, 1.0F);
+
+ writer.write(0, record(schema, inner));
+ writer.write(0, record(schema, null));
+ writer.write(0, record(schema, null));
+
+ Map<Integer, FieldMetrics<?>> metrics = metricsById(writer);
+ // the optional field is corrected
+ assertThat(metrics.get(2).nullValueCount()).isEqualTo(2);
+ assertThat(metrics.get(2).valueCount()).isEqualTo(3);
+ // the required field is left as the writer saw it: only the one populated
value
+ assertThat(metrics.get(3).nullValueCount()).isEqualTo(0);
+ assertThat(metrics.get(3).valueCount()).isEqualTo(1);
+ }
+
+ @Test
+ void nullStructAddsToNullsCountedByNestedField() {
+ Types.StructType struct = Types.StructType.of(optional(2, "d",
Types.DoubleType.get()));
+ Schema schema = new Schema(optional(1, "s", struct));
+
+ ParquetValueWriter<Record> writer = writerFor(schema);
+ Record present = GenericRecord.create(struct);
+ present.set(0, 2.0D);
Review Comment:
It is _okay_ to use the ordinal-based set method, but we prefer using
`setField` by name.
--
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]
