Copilot commented on code in PR #1094:
URL: https://github.com/apache/sedona-db/pull/1094#discussion_r3676222630
##########
python/sedonadb/tests/test_dataframe_ffi.py:
##########
@@ -121,3 +123,109 @@ def test_ffi_roundtrip(geoarrow_data, producer_sql,
consumer_sql):
result_over_ffi = sd_consumer.sql(consumer_sql).to_pandas()
pd.testing.assert_frame_equal(result_no_ffi, result_over_ffi)
+
+
+def test_filter_pushdown_into_ffi_producer(geoarrow_data):
+ path = geoarrow_data / "ns-water" / "files" /
"ns-water_water-point_geo.parquet"
+ skip_if_not_exists(path)
+
+ sd_producer = sedonadb.connect()
+ sd_consumer = sedonadb.connect()
+
+ # Producer exposes all columns without filtering
+ sd_producer.read_parquet(path).to_view("water_point")
+ df_producer = sd_producer.sql('SELECT "OBJECTID", "FEAT_CODE" FROM
water_point')
+
+ # Consumer applies a filter
+ sd_consumer.create_data_frame(df_producer).to_view("df_producer")
+ df_filtered = sd_consumer.sql('SELECT * FROM df_producer WHERE "OBJECTID"
< 50')
+
+ # Get the physical plan
+ plan_df = df_filtered.explain().to_pandas()
+ plan_text = "\n".join(plan_df["plan"].astype(str).tolist())
+
Review Comment:
`explain().to_pandas()` returns multiple plan sections
(logical/optimized/physical). Joining all `plan` rows can make the
`find("FilterExec")` / `find("ImportedSedonaCExec")` checks nondeterministic if
`FilterExec` appears in a different plan section before the physical plan.
Restrict `plan_text` to the `physical_plan` rows only.
This issue also appears on line 203 of the same file.
##########
c/sedona-extension/src/sedona_extension.h:
##########
@@ -229,27 +229,47 @@ struct SedonaCError {
void (*release)(struct SedonaCError* self);
};
-struct SedonaCExpr {
+/// \brief Non-owning view of an expression
+///
+/// This structure provides a read-only view into an expression without taking
+/// ownership. The lifetime of this view is tied to the underlying expression
+/// it references. The SedonaCExprView may be logical or physical...it is not
+/// typically executed but inspected for the purposes of pruning. The primary
+/// mechanism of recreating an expression is DataFusion Protobuf via the
+/// "datafusion_expr_proto" property; however, other types of queries may be
+/// supported by other properties in the future (e.g., rendering SQL or
+/// Substrait or extracting a query bounding box directly).
Review Comment:
The doc comment references a non-existent / mismatched property name
(`"datafusion_expr_proto"`). The implementation exports/consumes
`"datafusion_expr_protobuf"` (see `c/sedona-extension/src/expr.rs`). This
mismatch can mislead FFI consumers.
--
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]
