laskoviymishka commented on code in PR #2125:
URL: https://github.com/apache/iceberg-go/pull/2125#discussion_r4198396579
##########
table/internal/parquet_files.go:
##########
@@ -1188,7 +1190,8 @@ func normalizeWKBArrayReachable(ext array.ExtensionArray,
active []bool, mem mem
}
// accumulateGeoBounds extends the per-field bounding boxes with the WKB values
-// in this batch. Null rows are skipped; a malformed WKB value fails the write.
+// in this batch and tallies their null counts. Null rows are skipped; a
Review Comment:
This recovers null counts for top-level geo columns, but a geo field nested
in a struct/list/map gets the same logical type and loses its footer stats too,
and neither its bounds nor null counts are reconstructed here. That's safe
(absent, not wrong), but if the PR claims to close the last piece of #989 I'd
note the top-level-only limitation in a comment or the description, or file a
follow-up, so the half-restored state is intentional and visible.
##########
table/arrow_utils_test.go:
##########
@@ -3355,31 +3358,37 @@ func TestGeoTypeParquetRoundTrip(t *testing.T) {
name string
icebergType iceberg.Type
geoarrowMetaJSON string
+ parquetLogical schema.LogicalType
}{
{
name: "geometry_default_crs",
icebergType: iceberg.GeometryType{},
geoarrowMetaJSON:
`{"crs":"OGC:CRS84","crs_type":"authority_code"}`,
+ parquetLogical: schema.GeometryLogicalType{},
},
{
name: "geometry_srid_0",
icebergType: geomSRID0,
geoarrowMetaJSON: `{"crs":"0","crs_type":"srid"}`,
+ parquetLogical: schema.GeometryLogicalType{Crs:
"srid:0"},
},
{
name: "geography_srid_0",
icebergType: geogSRID0,
geoarrowMetaJSON:
`{"crs":"0","crs_type":"srid","edges":"spherical"}`,
+ parquetLogical: schema.GeographyLogicalType{Crs:
"srid:0", Algorithm: schema.GeographyEdgeSpherical},
Review Comment:
The round-trip cases cover geometry well but the only geography case is
`srid:0`. A default `GeographyType{}` (default CRS and algorithm) is the
likeliest place the mapping differs, since the Parquet spec omits the default
CRS, so I'd add a default-geography case. `srid:0` is also an unusual CRS to
pin; worth cross-checking the `srid:<id>` / omitted-OGC:CRS84 spelling against
a Java or DuckDB-written file so we know third-party readers agree on what we
emit.
##########
go.mod:
##########
@@ -38,7 +38,7 @@ require (
github.com/beltran/gohive v1.8.1
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
github.com/docker/docker v28.5.2+incompatible
- github.com/geoarrow/geoarrow-go v0.0.0-20260403143023-f54751c3e3a1
+ github.com/geoarrow/geoarrow-go v0.0.0-20261005150217-fc2b33c3141d
Review Comment:
This is an untagged pseudo-version dated today, and the bump changes on-disk
output (files now carry the GEOMETRY/GEOGRAPHY logical type). Pinning
format-affecting behavior to an unreleased commit means an upstream force-push
or a later CRS-mapping change silently shifts what we write. I'd depend on a
tagged geoarrow-go release if one carries this, or call out the pin in the PR
description if we have to take it as-is.
##########
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:
This writes the null count unconditionally, but `geoNullCounts` only has a
key for fields `accumulateGeoBounds` actually tallied. When that function hits
one of its guards and continues (`colIdx` past `NumCols`, the array isn't an
`ExtensionArray`, or the `wkbStorage` cast fails), the field never gets a key,
and the map read here returns 0. A definite 0 is worse than omitting the entry:
a reader prunes `IS NULL` and treats `NOT NULL` as always true, so a geo column
that silently fell through can drop real rows.
I'd only write entries we actually counted, e.g. tally into `geoNullCounts`
on the first successful cast and here do `if n, ok := w.geoNullCounts[fieldID];
ok { stats.NullValueCounts[fieldID] = n }`. Or, since every `geoCols` entry
comes from a `*geoarrow.WKBType` field, a failed cast is an invariant violation
rather than a skip, so returning an error there would be defensible too.
##########
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 {
Review Comment:
The accumulation this adds is cross-batch (`+=` across Write calls / row
groups), but no test writes more than one batch, so a regression like `=` for
`+=` would pass. I'd add a case that writes two batches with a small
`ParquetRowGroupLimitKey` and nulls spread across them, then asserts the total.
The counts-mode case passing also suggests the accumulator is created
regardless of mode; worth confirming the null tally isn't gated to modes that
only want bounds, since it's iterated off `geoAccs`.
##########
table/internal/parquet_files_test.go:
##########
@@ -3238,7 +3238,10 @@ func TestWriteDataFileGeoBounds(t *testing.T) {
assert.True(t, geomType.Equals(lit.Type()), "want geo type, got
%s", lit.Type())
assert.Equal(t, geoBoundBytes(5, 10),
lit.(iceberg.GeoLiteral).Value())
- // Null counts are still recorded for geo columns.
+ // Null counts are still recorded for geo columns. The Parquet
writer
+ // omits statistics for GEOMETRY/GEOGRAPHY columns, so these
come from
+ // the Arrow data rather than the footer.
+ assert.Equal(t, int64(0), df.NullValueCounts()[2])
Review Comment:
`NullValueCounts()[2]` returns 0 for a missing key as well as a real 0, so
this passes even if the write in `applyGeoBounds` never ran. I'd
`require.Contains(df.NullValueCounts(), 2)` before asserting the value, and
better still give the geometry column an actual null here so a non-zero count
proves it was computed from the Arrow data rather than defaulted.
##########
table/internal/parquet_files.go:
##########
@@ -517,6 +517,7 @@ type ParquetFileWriter struct {
geoCols []geoColumn
geoNormalizeCols []int
geoAccs map[int]*geoBoundsAccumulator
+ geoNullCounts map[int]int64
Review Comment:
Minor, but the null count is a second map keyed by the same field ID as
`geoAccs` and tallied in the same loop. Hanging it on `geoBoundsAccumulator` (a
`nulls int64` field) keeps the two concerns together and is what would let you
decouple null counting from the bounds iteration cleanly.
--
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]
