C-Loftus opened a new pull request, #1533: URL: https://github.com/apache/arrow-go/pull/1533
## Summary Follow-up to #1504. GEOMETRY and GEOGRAPHY column chunks now get a `ColumnMetaData.statistics` block that holds only `null_count`. min/max are still not written. The Parquet spec says min/max must not be written for types whose sort order is undefined, such as GEOMETRY and GEOGRAPHY. However, it also says writers SHOULD always write `null_count`, even for these types. arrow-go, like Arrow C++, left out the whole `Statistics` block for these types, so the footer had no null count at all. The Rust writer and DuckDB's writer both write a `Statistics` block holding only `null_count` for geometry columns, and this PR does the same. This change also lets DuckDB skip row groups using the bounding boxes from #1504. DuckDB v2.0.0 ignores a chunk's `geospatial_statistics` when `statistics` is unset ([`parquet_statistics.cpp`](https://github.com/duckdb/duckdb/blob/v2.0.0/extension/parquet/parquet_statistics.cpp), the early return before `TransformParquetStatistics`). So until now it read every row group of files arrow-go writes, even though their bounding boxes were correct. ## What this PR changes All changes are in `parquet/file/column_writer.go`: - **Setup:** when statistics are enabled for a GEOMETRY or GEOGRAPHY byte-array column, the writer counts that column's nulls. GEOMETRY still also gets the #1504 geospatial statistics. - **Counting nulls:** `WriteDefinitionLevels` counts every definition level below the maximum as a null. All write paths go through it: normal, spaced, Arrow offsets, and dictionary indices. This is the same "levels minus values" count the typed statistics use for other columns. - **`Close`:** calls `SetStats` with only `null_count` set. min, max, `min_value`, `max_value` and `distinct_count` stay unset. ## Points to review 1. **GEOGRAPHY gets a null count too.** It still gets no geospatial statistics (see #1504), but the spec recommends `null_count` for every column, and that number is easy to get right. If you'd rather keep this to GEOMETRY only, it's a one-line change. 2. **The null count is written even when the bounding box isn't.** #1504 drops the geospatial statistics for invalid WKB, for dictionary-index writes, and for chunks that are all null. The null count is still correct in all three cases, so the `Statistics` block is written anyway. 3. **The reader side is unchanged.** `ColumnChunkMetaData.StatsSet()` and `Statistics()` still report nothing for undefined-sort-order columns, so nothing in arrow-go starts trusting min/max for them. `GeoStatistics()` is also unchanged. 4. **Page-level statistics are not written** for these columns. Readers don't need them to use the chunk's geospatial statistics. 5. **Existing issue, out of scope:** data page v2 headers take `num_nulls` from the page statistics. Columns with an undefined sort order have no page statistics, so their v2 page headers record 0 nulls even when the page has nulls. This affects every such column, not only geometry, so it would be better fixed separately. ## Testing - **New test `TestGeoColumnNullCountStatistics`** in `parquet/file/column_writer_geo_test.go`. It decodes the footer thrift and checks exactly which fields are set. Each case runs with dictionary encoding on and off, and with data page v1 and v2: - GEOMETRY with nulls, written through `WriteBatch`, `WriteBatchSpacedWithError` and `WriteBatchArrow`: `null_count` is correct, the min/max fields are unset, and the geospatial statistics are unchanged. - GEOMETRY without nulls, a required GEOMETRY column, an all-null chunk, and invalid WKB. - GEOGRAPHY: `null_count` is written and there are no geospatial statistics. - Statistics disabled: neither block is written. - Reader round trip: `GeoStatistics()` still returns the box, and `StatsSet()` / `Statistics()` still report no typed statistics. - **DuckDB v2.0.0 with the spatial extension** (one-off script, not committed): with a two-row-group file and a filter on a box inside one group's extent, DuckDB now scans 1 of 2 row groups (2.52 MB → 1.27 MB read) for `&&`, `ST_Intersects` and `ST_Within`. On `main` it scans both. ## Related - DuckDB could also read `geospatial_statistics` when `statistics` is unset. That would be correct too, and it would help files from other writers that leave out the block. This PR works with DuckDB v2.0.0 as released. -- 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]
