uros-b commented on code in PR #17333:
URL: https://github.com/apache/iceberg/pull/17333#discussion_r3657070737
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetWriters.java:
##########
@@ -488,29 +489,53 @@ public void write(int repetitionLevel, byte[] bytes) {
}
}
+ private abstract static class GeospatialWriter<T> extends PrimitiveWriter<T>
{
+ private final ValueSizeFieldMetrics.Builder metricsBuilder;
+
+ private GeospatialWriter(ColumnDescriptor desc) {
+ super(desc);
+ this.metricsBuilder =
+ new
ValueSizeFieldMetrics.Builder(desc.getPrimitiveType().getId().intValue());
+ }
+
+ @Override
+ public void write(int repetitionLevel, T value) {
Review 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!
--
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]
