C-Loftus opened a new pull request, #1504: URL: https://github.com/apache/arrow-go/pull/1504
## Summary As discussed in [apache/iceberg-go#989](https://github.com/apache/iceberg-go/issues/989#issuecomment-6680086033), arrow-go currently doesn't write geospatial statistics for GEOMETRY row groups which limits query performance. The Parquet spec has a `GeospatialStatistics` field on `ColumnMetaData`. It holds a bounding box (X/Y, plus optional Z/M) and the list of geometry types seen in the column chunk. arrow-go could already read and write the GEOMETRY/GEOGRAPHY logical types, but it never filled in their statistics field. This PR makes the column writer compute those statistics for **GEOMETRY** columns as it writes them, and stores them in the footer. GEOGRAPHY columns are deliberately left out and intended as a follow up PR. Apologies in advance that this is a fairly long write up. However, I think the verbosity is useful for giving context for the code review. --- <details> <summary><strong>Context for Iceberg-go and the new end to end write path</strong></summary> When iceberg-go writes a geometry column, three repos are involved: - **iceberg-go** - Maps Iceberg `geometry`/`geography` types and their CRS to the geoarrow Arrow extension type (`table/arrow_utils.go`). - Computes the Iceberg manifest bounds and null counts for geo columns from the Arrow data, not from Parquet stats (`table/internal/geo_codec.go`, `geoBoundsAccumulator` in `table/internal/parquet_files.go`). - It has to, because arrow-go writes no `Statistics` block (min/max/null count) for types with an undefined sort order, like GEOMETRY and GEOGRAPHY. The spec only rules out min/max for these types. It says writers SHOULD always write `null_count`, and the Rust writer does. arrow-go (like Arrow C++) leaves the whole block out, so the footer has no null count for iceberg-go to read. - **geoarrow-go** - Defines the `geoarrow.wkb` Arrow extension type and maps it to and from the Parquet GEOMETRY/GEOGRAPHY logical types, including CRS and edges (`wkb.go`, `metadata.go`). - **arrow-go** - Defines the Parquet GEOMETRY/GEOGRAPHY logical types (`parquet/schema/logical_types.go`, released in v18.8.0). - Provides the pqarrow hooks that let an extension type pick its Parquet logical type on write and read (`ExtensionCustomParquetType`, `ExtensionCustomArrowReadType`, released in v18.8.0). - **This PR:** computes the column chunk `GeospatialStatistics` (bbox and geometry types) while writing, and stores them in the footer. A write flows like this: ``` Iceberg schema → geoarrow.NewWKBType(...) (geoarrow-go; Binary or LargeBinary storage) → pqarrow.NewFileWriter (arrow-go) → ParquetLogicalType() hook (geoarrow-go) → column is GEOMETRY(crs) / GEOGRAPHY(crs, algo) → columnWriter sees GeometryLogicalType (arrow-go, NEW) → builds GeoStatistics while writing → Close() → SetGeoStatistics → footer (arrow-go, NEW) ``` On the read side, any reader can call `rdr.MetaData().RowGroup(i).ColumnChunk(j).GeoStatistics()`. </details> ## What this PR changes - **New WKB parser** (`parquet/internal/geospatial/bounder.go`): reads WKB bytes and tracks the min/max of each dimension plus the geometry types seen. No geometry library needed. - **New `GeoStatistics` type** (`parquet/metadata/geo_statistics.go`): wraps the parser and converts to/from the thrift footer struct. - **Reader and writer hooks** (`parquet/metadata/column_chunk.go`): `ColumnChunkMetaData.GeoStatistics()` to read, `ColumnChunkMetaDataBuilder.SetGeoStatistics()` to write. - **Column writer** (`parquet/file/column_writer.go`): creates the statistics for GEOMETRY columns when stats are enabled, and writes them to the footer on `Close()`. - **Write paths** (`column_writer_types.gen.go.tmpl` + generated file, `byte_array_column_writer_arrow.go`): every ByteArray write path feeds values to the statistics. `WriteDictIndices` invalidates them instead (item 3). - **Tests:** `bounder_test.go` (WKB edge cases), `geo_statistics_test.go` (thrift round trip and update paths), `column_writer_geo_test.go` (end to end through the writer and reader). ## Non-obvious logic/ points to align on before merging I've tried to go with a sane default for all of these, but figured it's best practice to be explicit. ### 1. GEOGRAPHY columns get no statistics The spec allows GEOGRAPHY statistics, but computing them correctly needs fairly complex geodesic math. My plan is to leave this out of scope and revisit as a follow-up if there is demand among users. Reading is left out too: `GeoStatistics()` returns nil for GEOGRAPHY columns, even when another tool (such as DataFusion) wrote statistics for them. Those boxes can wrap across the antimeridian (`xmin > xmax`), which the new type doesn't represent. Reading GEOGRAPHY statistics, including wraparound boxes, can be part of the same follow-up. ### 2. A single bad value drops the statistics for the whole column chunk If any value isn't valid WKB, or has a NaN coordinate (other than a fully-NaN empty point), the whole chunk's statistics are marked invalid and nothing is written. That's safe, since readers can never prune wrongly, but it's stricter than some other tools. **Alternative:** skip only the bad value. That keeps pruning, at the cost of a bbox that doesn't cover every row (the spec doesn't say what to do here). ### 3. Arrow dictionary arrays invalidate the statistics `WriteDictIndices` (used by pqarrow for `Dictionary<…, binary>` arrays) only receives integer indices; the dictionary was already given to the encoder. Rather than decode the dictionary, the PR invalidates the statistics, and that sticks for the rest of the chunk. iceberg-go and geoarrow-go never produce dictionary arrays of WKB, so this doesn't affect them. ### 4. Z/M bounds are written if *any* geometry has Z/M If one geometry in the chunk has Z, the footer gets Z bounds that cover only those geometries. That matches the reference writer. iceberg-go's manifest only emits Z/M when *every* geometry has them, because Iceberg's bound semantics are different. Both are right for their own format, but the footer bbox and the Iceberg manifest bounds for the same file can differ on Z/M. Should be noted that this isn't bug in a cross-check test. ### 5. Only top-level geometry types are recorded `geospatial_types` lists the type of each *row*: a GeometryCollection containing a Point Z records `[7]`, not `[7, 1001]`. Coordinates of nested children still count toward the bounds, so that example produces Z bounds while the only type listed is 2D. This matches the spec's "type codes of all instances" wording, where an instance is a row. ### 6. The parser is lenient in some places These are accepted and not validated. Tests don't pin any of this behavior, so tightening them later is easy. - **EWKB** (the PostGIS Z/M/SRID flags) is accepted and normalized to ISO codes. The SRID is skipped. The spec says values are ISO WKB, but accepting EWKB costs nothing and handles files written by PostGIS-style tools. - **Multi\* child types aren't checked:** a MultiPoint that contains a LineString is accepted. - **Child dimensions aren't checked against the parent's:** a Point Z inside a 2D collection is accepted (see 5). - **±Inf coordinates** are accepted and end up in the bbox. ### 7. Chunks with no written geometries store nothing `toThrift` returns nil when no non-null geometry was seen, so all-null row groups get no `GeospatialStatistics`. The reference writer stores an empty struct (no types, no bbox). Both mean "no information" to a reader, so I think it is fine. ### 8. Public API surface New exported names in `parquet/metadata`: the `GeoStatistics` type and its methods, `NewGeoStatistics`, `UpdateGeoStatisticsFromArrowOffsets` (a generic function, because Go methods can't have type parameters), and `ColumnChunkMetaData.GeoStatistics()` / `ColumnChunkMetaDataBuilder.SetGeoStatistics()`. The `Update*`, `Merge`, `Reset` and `Invalidate` methods mostly exist for the writer. It's possible that fewer public methods may be desired. For example, readers only need the accessors, so the writer-only methods could be made unexported. `Merge` is unused today, but it's the obvious way to union row groups into a file-level box. ### 9. Write-time cost / benchmarking Statistics are on by default, so every GEOMETRY write now parses each WKB value once. The parser doesn't allocate per value and stops at the first error, but there's no benchmark yet. Callers who don't want the cost can turn statistics off for the column with `parquet.WithStatsFor(path, false)`. ### 10. Minor Geometry logic overlap in both arrow-go and geoarrow-go Both repos now deal with geometry, but at different layers: - **arrow-go works at the Parquet layer.** It defines the GEOMETRY/GEOGRAPHY logical types that describe a column in the file footer (CRS, edge algorithm). With this PR, it also has a small WKB parser that reads every coordinate to compute the bounding box and geometry types for the statistics. - **geoarrow-go works at the Arrow layer.** It defines the in-memory extension types (`geoarrow.wkb` and native types like point and polygon), with CRS and edges stored as Arrow metadata. It implements the pqarrow hooks that map its types to arrow-go's Parquet logical types. For WKB it only reads the 5-byte header (geometry type and dimension), and it uses go-geom when full decoding is needed. arrow-go has its own parser instead of reusing geoarrow-go for three reasons: - geoarrow-go imports arrow-go, so arrow-go can't import geoarrow-go without a dependency cycle. - The statistics are computed in the Parquet column writer, which only sees raw bytes and the Parquet logical type, not the Arrow extension type. Code that writes with `file.ByteArrayColumnChunkWriter` directly, without pqarrow or geoarrow-go, still gets statistics. - The parser only scans coordinates, so arrow-go doesn't need to depend on go-geom. ## Testing - `go test ./parquet/...` passes, with `PARQUET_TEST_DATA` and `ARROW_TEST_DATA` pointed at the submodules (`pqarrow` panics without them, unrelated to this PR). - `go generate` for `column_writer_types.gen.go` produces no diff, so the generated file matches the template. - **Cross-check against the reference files** in `parquet-testing/data/geospatial/` (one-off, not committed). The values were decoded and rewritten through this writer, and the footer stats compared: - `geospatial.parquet`: 30 of 31 row groups match exactly, covering every geometry type × XY/XYZ/XYM/XYZM, collections, empty geometries and nulls. The one difference is the all-null row group (item 7). - `crs-default`, `crs-srid`, `crs-projjson`: match. - `geospatial-with-nan.parquet`: differs on purpose (item 2). - **Iceberg end-to-end:** geoarrow-go (`fc2b33c`) and iceberg-go (`b1206c6`) pass their full test suites with a `replace` pointing at this branch. A one-off iceberg-go test that writes through `WriteDataFile` confirmed: GEOMETRY footer stats match the manifest X/Y bounds; GEOGRAPHY and all-null columns get no footer stats; mixed `POINT Z` + `POINT` gives footer Z bounds while the manifest is XY only (item 4). -- 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]
