zeroshade commented on code in PR #2125:
URL: https://github.com/apache/iceberg-go/pull/2125#discussion_r4198585163
##########
table/internal/parquet_files.go:
##########
@@ -1207,6 +1210,7 @@ func (w *ParquetFileWriter) accumulateGeoBounds(batch
arrow.RecordBatch) error {
if !ok {
continue
}
+ w.geoNullCounts[gc.fieldID] += int64(storage.NullN())
Review Comment:
Only top-level `geoCols` get a null tally, but `ParquetLogicalType()` also
annotates nested WKB leaves, so they lose their footer stats too. For a
`struct<g: geometry>` with one null `g` in 6 rows, main records
`null_value_counts[4] = 1`. This branch records no entry.
`FileToDataFile`/`AddFiles` and the metadata-only `DataFile` builder call
`DataFileStatsFromMeta` without `applyGeoBounds`, so they lose geo null counts
for files carrying the new annotation as well. Results stay correct (a missing
count only costs pruning), but it's a metrics regression against main. Either
extend the tally to nested geo leaves (counting rows where the parent is null,
as Parquet's leaf `null_count` does), or scope it out on purpose: pin the
current behaviour in a test and note it next to TODO(#992).
##########
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:
**Blocking.** Losing the footer stats breaks more than null counts.
`DataFileStatsFromMeta` checks `invalidateCol` *before* it adds
`colSizes[fieldID]`/`valueCounts[fieldID]`. Every `GEOMETRY`/`GEOGRAPHY` chunk
now reports `StatsSet() == false` (sort order UNKNOWN), so the geo column is
invalidated in row group 0 and the later row groups add nothing. Any geo file
with more than one row group (default `write.parquet.row-group-limit` is
1,048,576 rows) records `value_counts`/`column_sizes` for row group 0 only, and
this block puts the whole-file null count next to that partial value count.
That pairing gives wrong answers, not just wrong metadata. I wrote 6 rows
(first two geo values null) through `newDataFileWriter`/`writeFile` with
`write.parquet.row-group-limit=2`, which gives 3 row groups:
- main: `value_counts` = 6 for `geom`/`geog`
- this branch: `value_counts` = 2, `null_value_counts` = 2, `column_sizes`
from row group 0 only
`containsNullsOnly` is now true, so:
- the inclusive evaluator prunes the file for `NotNull(geog)`
- `VisitBBoxIntersects` prunes on the same check
- the strict evaluator returns rows-must-match for `IsNull(geog)`, so
`Delete(IsNull(...))` drops the whole file, 4 non-null rows included
Fix: in `DataFileStatsFromMeta`, move the two lines that add
`colSizes[fieldID]` and `valueCounts[fieldID]` above the `invalidateCol`
short-circuit. PyIceberg's `data_file_statistics_from_parquet_metadata` adds
sizes and value counts for every row group before it looks at stats. Java's
`ParquetMetrics.counts` never reports a partial sum either: it drops the
field's counts if any chunk lacks stats. With that reorder the repro gives
`value_counts` = 6 and correct evaluator results, and `go test ./table/internal
-run 'Stats|Geo|RowGroup'` still passes. Please add a multi-row-group
regression test (nulls in the first row group) that asserts `value_counts`
equals the record count and that `NotNull` isn't pruned.
##########
table/internal/parquet_files_test.go:
##########
@@ -3257,6 +3260,13 @@ func TestWriteDataFileGeoBounds(t *testing.T) {
df := writeWithGeomMode(t, tt.mode)
assert.NotContains(t, df.LowerBoundValues(), 2,
"geometry lower bound must be omitted for %s", tt.mode.Typ)
assert.NotContains(t, df.UpperBoundValues(), 2,
"geometry upper bound must be omitted for %s", tt.mode.Typ)
+
+ // Counts mode still records the geometry null count;
none records nothing.
+ if tt.mode.Typ == internal.MetricModeCounts {
+ assert.Equal(t, int64(0),
df.NullValueCounts()[2])
Review Comment:
Neither geometry row is null, so `df.NullValueCounts()[2]` reads `0` whether
or not a count was recorded (missing map key), and this branch can't fail. I
changed `applyGeoBounds` to skip `MetricModeCounts` entirely and
`TestWriteDataFileGeoBounds` still passed. The full-mode check at L3244 has the
same problem. Assert presence, or make one geometry row null so the expected
count is non-zero.
```suggestion
require.Contains(t, df.NullValueCounts(), 2,
"counts mode must record the geometry null count")
assert.Equal(t, int64(0),
df.NullValueCounts()[2])
```
--
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]
