[PR] fix(rust/sedona-geoparquet): Fix native GeoParquet pruning [sedona-db]

["reactima (via GitHub)" <gi...@apache.org>]

reactima opened a new pull request, #1430:
URL: https://github.com/apache/sedona-db/pull/1430

   Hi, looks like there is a panic bug in mapping Parquet leaves to Arrow fields. 
   
   ### Problem
   
   Any filtered query panics on a GeoParquet file when the geometry column uses Parquet's **native Geometry type** and a **nested column** (a struct or a list) comes before it. The filter does not have to be spatial:
   
   ```sql
   CREATE EXTERNAL TABLE c1 STORED AS PARQUET
     LOCATION 'https://data.source.coop/tge-labs/s2-stac-geoparquet/sentinel-2-c1-l2a/year=2024/items.parquet';
   SELECT count(*) FROM c1 WHERE _tile = '32TNQ';
   ```
   
   ```
   index out of bounds: the len is 52 but the index is 56
   ```
   
   ### Where this code runs
   
   For a query with a `WHERE` clause, the opener in `rust/sedona-geoparquet/src/file_opener.rs` tries to skip row groups that cannot match. For native Geometry columns it does this:
   
   ```
   filter_access_plan_using_native_geostats()
     ├─ top_level_column_indices()            ← which Parquet columns to read (changed here)
     └─ for each row group:
          row_group_native_geo_stats()        ← one GeoStatistics per entry of that list
          TableGeoStatistics::try_from_stats_and_schema()   ← looks the geometry up in the list
          spatial_filter.evaluate()           ← skip the row group, or keep it
   ```
   
   ### Two ways to number columns
   
   - **Arrow** (`file_schema`) numbers **top-level fields**. A struct counts as one field.
   - **Parquet** stores only **leaves**, the primitive columns at the bottom of the tree. `row_group.column(i)` and `schema_descr.num_columns()` count leaves, so a struct with four children is four leaves.
   
   The schema in the new test:
   
   | Arrow field | Name | Parquet leaves (path) |
   |---|---|---|
   | 0 | `not_geo` | 0: `not_geo` |
   | 1 | `bbox` (struct) | 1: `bbox.xmin`, 2: `bbox.ymin`, 3: `bbox.xmax`, 4: `bbox.ymax` |
   | 2 | `some_geo` | 5: `some_geo` |
   
   ### What the list must look like
   
   `try_from_stats_and_schema()` reads the statistics by **Arrow field position**:
   
   ```rust
   for i in schema.geometry_column_indices()? {          // i = Arrow field index of a geometry column
       stats_map.insert(..., column_stats[i].clone());   // so one entry per Arrow field is required
   }
   ```
   
   So `top_level_column_indices()` must return **one Parquet leaf number per Arrow field, in field order**. Here that means three entries, with entry 2 pointing at leaf 5.
   
   ### The old loop
   
   ```rust
   for (i, col) in schema_descr.columns().iter().enumerate() {   // every leaf
       let path_vec = col.path().parts();
       if path_vec.len() == 1 {                                   // keep only one-part paths
           top_level_indices.push(i);
       }
   }
   ```
   
   A nested field's leaves have multi-part paths (`bbox.xmin`, `bbox.list.element`), so the field contributes **nothing**. The result is `[0, 5]`: two entries for three fields. `some_geo` is Arrow field 2, so the lookup reads `column_stats[2]`, past the end of the list, and panics. When the index happens to stay in range, it reads another field's statistics instead; with a single geometry column, that silently disables native pruning.
   
   ### The new loop
   
   ```rust
   for i in 0..schema_descr.num_columns() {                                // (1)
       if schema_descr.get_column_root_idx(i) == top_level_indices.len() { // (2)
           top_level_indices.push(i);                                      // (3)
       }
   }
   ```
   
   1. **Walk every leaf** in storage order.
   2. `get_column_root_idx(i)` is the **top-level field that leaf `i` belongs to**. `top_level_indices.len()` is the number of fields that already have an entry, which is the **next field still waiting for one**. They are equal only at **the first leaf of the next field**.
   3. **Record that leaf** for that field. The list grows by one, so the field's remaining leaves no longer match.
   
   Trace on the test schema:
   
   | `i` | Path | Root (field) | `len()` before | Equal? | List after |
   |---|---|---|---|---|---|
   | 0 | `not_geo` | 0 | 0 | yes, push 0 | `[0]` |
   | 1 | `bbox.xmin` | 1 | 1 | yes, push 1 | `[0, 1]` |
   | 2 | `bbox.ymin` | 1 | 2 | no | `[0, 1]` |
   | 3 | `bbox.xmax` | 1 | 2 | no | `[0, 1]` |
   | 4 | `bbox.ymax` | 1 | 2 | no | `[0, 1]` |
   | 5 | `some_geo` | 2 | 2 | yes, push 5 | `[0, 1, 5]` |
   
   One entry per Arrow field, and entry 2 is leaf 5 (`some_geo`).
   
   
   or more verbose 
   
   ```rust
   let mut top_level_indices = Vec::new();
   let schema_descr = parquet_metadata.file_metadata().schema_descr();
   for i in 0..schema_descr.num_columns() {
       // Leaves are grouped by top-level field; keep the first leaf of each field
       let field = schema_descr.get_column_root_idx(i);
       let is_first_leaf_of_field = i == 0 || schema_descr.get_column_root_idx(i - 1) != field;
       if is_first_leaf_of_field {
           top_level_indices.push(i);
       }
   }
   ```
   
   
   
   ### What happens with each entry
   
   `row_group_native_geo_stats()` turns each leaf number into statistics with `row_group.column(leaf).geo_statistics()`:
   
   - **The geometry's leaf** has native statistics, so its entry is a real bounding box, which is used for pruning.
   - **A nested field's first leaf** (`bbox.xmin`, a double) has none, so its entry is `GeoStatistics::unspecified()`. That means "might match", so it never causes a row group to be skipped. It only keeps the positions aligned.
   
   ### Why the loop can rely on leaf order
   
   `SchemaDescriptor::new()` in the parquet crate (`parquet/src/schema/types.rs`) walks the top-level fields in order and builds each one depth-first, pushing `leaf_to_base.push(root_idx)` for every leaf. `get_column_root_idx(i)` returns `leaf_to_base[i]`. So across the leaves the root numbers look like `0, 1, 1, 1, 1, 2, …`: they never go down, and a field's leaves sit next to each other. That is what makes the condition fire exactly once per field.
   
   ### Assumptions
   
   - Like the old code, this assumes the file's top-level fields come in `file_schema` order.
   - Every top-level field has at least one leaf. Parquet writers refuse empty structs; arrow-rs errors with "Parquet does not support writing empty structs".
   - If a struct's first child were a geometry, its statistics would sit at the struct's position. `try_from_stats_and_schema()` reads only top-level geometry fields, so that entry is never used.
   - No signatures or callers change.
   
   ### Tests
   
   - New test `native_geo_stats_nested_column_before_geometry`: the schema above. It asserts `[0, 1, 5]`, the aligned statistics, and that `try_from_stats_and_schema()` resolves the geometry. It fails on main with `left: [0, 5]`, `right: [0, 1, 5]`.
   - `cargo test -p sedona-geoparquet --lib file_opener`: 28 passed.
   - A self-contained repro, on a file SedonaDB writes itself, with the same shape as the test (`id`, a `bbox` struct, then a native `geometry`):
   
     ```sql
     COPY (SELECT 1 AS id,
                  named_struct('xmin', 0.0, 'ymin', 0.0, 'xmax', 1.0, 'ymax', 1.0) AS bbox,
                  ST_SetSRID(ST_GeomFromWKT('POINT (0.5 0.5)'), 4326) AS geometry)
       TO '/tmp/nested_before_geom.parquet' STORED AS PARQUET OPTIONS ('geoparquet_version' '2.0');
     CREATE EXTERNAL TABLE t STORED AS PARQUET LOCATION '/tmp/nested_before_geom.parquet';
     SELECT count(*) FROM t WHERE id = 1;
     ```
   
     On main it panics with `index out of bounds: the len is 2 but the index is 2` (at `rust/sedona-expr/src/spatial_filter.rs:615`). With this change, it returns 1.
   
   The native statistics path was added in #561.


-- 
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: issues-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(rust/sedona-geoparquet): Fix GeoParquet leaves mapping to Arrow fields [sedona-db]

["paleolimbot (via GitHub)" <gi...@apache.org>]

paleolimbot merged PR #1430:
URL: https://github.com/apache/sedona-db/pull/1430


-- 
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: issues-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org