fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader

[paleolimbot]

This PR fixes a workaround we have for metadata getting dropped in the Parquet implementation's try_pushdown_projection. Because we advertize the file schema with different field metadata than the inner opener, we need to wrap the expressions (or UDF calls fail because there is no type metadata).

I had thought the projection expressions we see had already been normalized to the file schema; however, it appears that these schemas are stacked (we might get more than one projection pushed down, and while the underlying implementation handles the accumulated schemas, we can't look to see what that is. There is a chance this approach will accidentally add metadata to a field whose metadata had been stripped and given the same name.

This PR applies our metadata wrapping once when constructing the file source, then lets the inner implementation handle the accumulating of projections.

Closes #1115.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes GeoParquet projection pushdown wrapping so that Column expressions are matched to the file schema by name (not index), avoiding panics and preserving extension metadata needed by UDFs (regression for #1115).

Changes:

• Update wrap_expr_columns to resolve fields via file_schema.column_with_name(column.name()) and skip wrapping for columns not present in the file schema.
• Add Rust regression/unit tests covering derived/intermediary columns and geometry wrapping behavior.
• Add a Python regression test reproducing the Arrow materialization panic scenario with ST_* accessors plus extra non-geometry columns.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
rust/sedona-geoparquet/src/format.rs	Fixes schema lookup in wrap_expr_columns and adds targeted Rust regression/unit tests.
python/sedonadb/tests/io/test_parquet.py	Adds an end-to-end Python regression test that exercises the failure mode from #1115.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

🟡 Changes recommended

The Rust changes introduce a likely projection semantics regression (defaulting to column 0) and still contain an unchecked schema field index access that can panic on intermediary projections.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

rust/sedona-geoparquet/src/format.rs:707

• wrap_expr_columns uses file_schema.field(index) without checking bounds. If a Column expr refers to an intermediary/stacked projection column (index >= file_schema field count), this will panic (the same class of failure as the reported "index out of bounds").

        if let Some(column) = node.as_any().downcast_ref::<Column>() {
            let index = column.index();
            let field = file_schema.field(index);
            // Only wrap columns that have extension metadata
            if field.metadata().contains_key("ARROW:extension:name") {
                let field: FieldRef = Arc::new(field.clone());
                let wrapped = Arc::new(MetadataPreservingColumn::new(column.clone(), field));

rust/sedona-geoparquet/src/format.rs:1143

• This test/docstring says it covers a "geometry accessor", but the query only selects the raw geometry column (no ST_*/accessor expression). Renaming it avoids giving a false sense of coverage.

    /// Integration test for projection with multiple derived columns plus geometry accessor
    /// Regression test for https://github.com/apache/sedona-db/issues/1115
    #[tokio::test]
    async fn projection_with_derived_columns_and_geometry_accessor() {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[paleolimbot]

This is a drive by fix...pytest objects to this file and won't collect this test in VSCode if it exists

[Copilot]

🟡 Changes recommended

The added Python regression test doesn’t execute its CREATE VIEW statements, and the Rust-side wrapping logic still has an unchecked schema index access that can panic.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

python/sedonadb/tests/io/test_parquet.py:848

• Same as above: this CREATE VIEW statement needs to be executed, otherwise the view may not exist for the subsequent query.

    con.sql("CREATE OR REPLACE VIEW t AS SELECT *, ST_IsEmpty(geometry) AS _e FROM g1")

python/sedonadb/tests/io/test_parquet.py:845

• Context.sql() is lazy (returns a DataFrame); without calling .execute() the CREATE VIEW statements may never run, so the later SELECT * FROM t can fail because g1/t were never created.

This issue also appears on line 848 of the same file.

    con.sql("""
        CREATE OR REPLACE VIEW g1 AS
        SELECT 'a' AS c1, 'b' AS c2, ST_SetSRID(ST_GeomFromWKB(geometry), 4326) AS geometry
        FROM raw
    """)

rust/sedona-geoparquet/src/format.rs:1164

• This regression test only asserts the number of output columns; it doesn't verify that the geometry field's extension metadata survived projection evaluation (which is the bug being fixed). Adding an explicit metadata assertion would make the test actually detect the regression.

        // projection pushdown tried to wrap columns with indices beyond the
        // file schema's bounds
        let batches = df.collect().await.unwrap();
        assert!(!batches.is_empty());
        assert_eq!(batches[0].num_columns(), 3);

rust/sedona-geoparquet/src/format.rs:1145

• The test name mentions a "geometry accessor", but the query doesn't call any accessor/UDF; renaming to reflect what's actually being asserted (derived columns + metadata preservation) will make the intent clearer.

    async fn projection_with_derived_columns_and_geometry_accessor() {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[james-willis]

Is it ok to silently not wrap? When do we expect this case to hit?

[paleolimbot]

That's a great point...it's not expected to fail but I added a deferred error mechanism to make sure we know if it does

[james-willis]

Isn't this just a copy of what is already in ParquetSource.new? When would we hit this case?

https://github.com/apache/datafusion/blob/branch-52/datafusion/datasource-parquet/src/source.rs#L308

[paleolimbot]

We shouldn't hit this case, but if that gets removed on upgrade and set to None at least it's still correct.

[james-willis]

field from table schema right? thats kind of the idea here that the table might have type metadata that isnt correctly reflected in the file?

[paleolimbot]

There's a difference between the table schema that we have access to and the table schema the Parquet opener uses to evaluate projections / filters. This is not my favourite workaround...the idea is to "just wrap" the Parquet DataFusion implementation but it's accumulated quite a bit of workarounds and it's not as clean as I would like it to be.

All of this was rewritten on the Parquet end for each of the 53, 54, and 55 releases so we will have to redo these workarounds a few times in our future 😮‍💨

[james-willis]

This would get swallowed by the match at line 418.

Really wouldn't expect this to even happen since schema and expr come from the same place

[paleolimbot]

I added the deferred error mechanism to make sure this gets caught

[james-willis]

should we assert the metadata survives as well?

[paleolimbot]

I added this to the test!