Parquet: Track avg_value_size_in_bytes for Geo types - #17333
Conversation
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
Thank you @huan233usc, left just one comment and adding @szehon-ho for further review
| } | ||
|
|
||
| @Override | ||
| public void write(int repetitionLevel, T value) { |
There was a problem hiding this comment.
The Spark GeospatialWriter.write() override is a genuinely new code path (calls toWkb(value) then addValueSize(wkb.length)), but no test asserts its avgValueSizeInBytes output; the referenced round-trip test only checks WKB equality/null-ness. The generic writer's metric is directly asserted (TestParquetValueWriters.geospatialValueSizeMetricsExcludeNulls), so the shared accumulation logic is covered, but the Spark writer's size measurement is unexercised. Please consider adding an assertion on the Spark path before merge, also - @szehon-ho please review this PR for Geo, thank you!
Adds TestSparkParquetWriter.testGeospatialAvgValueSizeMetrics, which exercises the Spark GeospatialWriter.write() path (toWkb + addValueSize) that the existing WKB round-trip test did not cover, asserting avgValueSizeInBytes over non-null geometry/geography values. Mirrors the generic writer's TestParquetValueWriters assertion.
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
File: parquet/.../ParquetMetrics.java (metrics assembly), core/.../FieldStatsStruct.java
Writers correctly accumulate WKB size and FieldStatsStruct.fromFieldMetrics now copies avgValueSizeInBytes, but:
ParquetMetrics.metrics() still builds a Metrics object with no avg field, so DataFile metrics drop it
fromFieldMetrics has no production callers (tests only)
So this does not yet produce avg_value_size_in_bytes in manifests/content stats.
Are we planning to do in follow up?
|
@szehon-ho Yes, this is planned for a separate follow-up PR. This PR is scoped to collecting and plumbing the average WKB value size through |
|
Merged, thanks @huan233usc and @uros-b for additional review! |
3 participants
Follow-up to #17313.
This collects the serialized WKB size for geometry and geography values written to Parquet and carries the average over non-null values through
FieldMetricsinto v4FieldStats. Both the generic Parquet writer and Spark 4.1 writers populate the metric.This is based directly on
mainand is independent of the bounding-box work in #17161.Tests:
./gradlew :iceberg-core:test --tests org.apache.iceberg.TestValueSizeFieldMetrics --tests org.apache.iceberg.TestFieldStatsStruct./gradlew :iceberg-parquet:test --tests org.apache.iceberg.parquet.TestParquetValueWriters./gradlew :iceberg-data:test --tests org.apache.iceberg.parquet.TestParquetMetrics.testMetricsForGeospatialTypes./gradlew :iceberg-spark:iceberg-spark-4.1_2.13:test --tests org.apache.iceberg.spark.data.TestSparkParquetWriter.testGeospatialWkbRoundTrip./gradlew :iceberg-core:revapi :iceberg-parquet:revapi