C-Loftus commented on code in PR #2125:
URL: https://github.com/apache/iceberg-go/pull/2125#discussion_r4214117977
##########
table/internal/parquet_files.go:
##########
@@ -1268,17 +1272,29 @@ func (w *ParquetFileWriter) Abort() error {
return errors.Join(closeErr, removeErr)
}
-// applyGeoBounds injects the WKB single-point bounds accumulated during the
-// write into the file statistics, so they flow through ToDataFile into the
-// manifest entry like any other typed bound.
+// applyGeoBounds injects the WKB single-point bounds and null counts
+// accumulated during the write into the file statistics, so they flow through
+// ToDataFile into the manifest entry like any other typed bound.
+//
+// Parquet GEOMETRY/GEOGRAPHY columns have an undefined sort order, so the
+// Parquet writer omits their column statistics entirely and
+// DataFileStatsFromMeta cannot recover null counts for them.
func (w *ParquetFileWriter) applyGeoBounds(stats *DataFileStatistics) error {
for fieldID, acc := range w.geoAccs {
// Honor the column's metrics mode: a column the caller never
registered
- // (missing key) or one set to counts/none does not record
bounds. Check
- // presence first — a missing key yields a zero-value mode of
"" that would
- // otherwise fall through and write bounds the caller asked to
skip.
+ // (missing key) or one set to none records nothing, and counts
records
+ // no bounds. Check presence first — a missing key yields a
zero-value
+ // mode of "" that would otherwise fall through and write
bounds the
+ // caller asked to skip.
sc, ok := w.info.StatsCols[fieldID]
- if !ok || sc.Mode.Typ == MetricModeNone || sc.Mode.Typ ==
MetricModeCounts {
+ if !ok || sc.Mode.Typ == MetricModeNone {
+ continue
+ }
+ if stats.NullValueCounts == nil {
+ stats.NullValueCounts = make(map[int]int64)
+ }
+ stats.NullValueCounts[fieldID] = w.geoNullCounts[fieldID]
Review Comment:
Went with returning an error: every geoCols entry comes from a
*geoarrow.WKBType field, so those guards now fail the write instead of
skipping, and a recorded null count always covers each batch. The separate map
is also gone and the count lives on geoBoundsAccumulator.nulls. Think that
should probably work
--
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]
