jiayuasu commented on code in PR #1216:
URL: https://github.com/apache/sedona-db/pull/1216#discussion_r3938946786
##########
rust/sedona-functions/src/executor.rs:
##########
@@ -19,14 +19,37 @@ use std::iter::zip;
use arrow_array::ArrayRef;
use arrow_schema::DataType;
use datafusion_common::cast::{as_binary_array, as_binary_view_array,
as_struct_array};
+use datafusion_common::config::ConfigOptions;
use datafusion_common::error::Result;
use datafusion_common::{DataFusionError, ScalarValue};
use datafusion_expr::ColumnarValue;
-use sedona_common::sedona_internal_err;
-use sedona_geometry::wkb_header::WkbPointLayout;
+use sedona_common::{option::SedonaOptions, sedona_internal_err};
+use sedona_geometry::{bounds::WkbBounder2D, types::Edges,
wkb_header::WkbPointLayout};
use sedona_schema::datatypes::SedonaType;
use wkb::reader::{read_wkb, Wkb};
+/// Resolve the session bounder for a geometry/geography argument.
+pub(crate) fn bounder_for_arg_type(
+ arg_type: &SedonaType,
+ config_options: Option<&ConfigOptions>,
+ function_name: &str,
+) -> Result<Box<dyn WkbBounder2D>> {
+ let edges = match arg_type {
+ SedonaType::Wkb(edges, _) | SedonaType::WkbView(edges, _) => *edges,
+ SedonaType::Arrow(DataType::Null) => Edges::Planar,
+ _ => return sedona_internal_err!("Expected geometry or geography, got
{arg_type:?}"),
+ };
+ let factory = config_options
Review Comment:
Could we register the bounder in `new_from_context()` too? With the
`sedona/s2geography` feature enabled, this test now fails:
```rust
#[tokio::test]
async fn geography_bounds_default_context() {
let ctx = sedona::context::SedonaContext::new();
ctx.ctx
.sql("SELECT ST_XMin(ST_GeogFromText('POINT (1 2)'))")
.await.unwrap()
.collect().await.unwrap();
}
```
The error is `ST_XMin() requires a bounder for Spherical edges, which is not
registered in this session`. `new_from_context(SessionContext::new())` has the
same problem. `new_local_interactive()` works because it installs the bounder.
##########
rust/sedona-functions/src/st_envelope.rs:
##########
@@ -82,18 +80,29 @@ impl<T: WkbBounder2D + Default> SedonaScalarKernel for
STEnvelope<T> {
&self,
arg_types: &[SedonaType],
args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ self.invoke_batch_from_args(arg_types, args, &WKB_GEOMETRY, 0, None)
+ }
+
+ fn invoke_batch_from_args(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ _return_type: &SedonaType,
+ _num_rows: usize,
+ config_options: Option<&ConfigOptions>,
) -> Result<ColumnarValue> {
let executor = WkbBytesExecutor::new(arg_types, args);
let mut builder = BinaryBuilder::with_capacity(
executor.num_iterations(),
WKB_MIN_PROBABLE_BYTES * executor.num_iterations(),
);
- let mut bounder = T::default();
+ let mut bounder = bounder_for_arg_type(&arg_types[0], config_options,
"ST_Envelope")?;
Review Comment:
Is geography support meant to survive a native UDF export/import roundtrip?
I reproduced a failure at the Rust FFI boundary; the equivalent Python example
is:
```python
import sedonadb
from sedonadb.udf import sedona_native_scalar_udf
con = sedonadb.connect()
capsules = con.funcs.st_envelope.__sedonadb_scalar_udf__()
con.register(sedona_native_scalar_udf(capsules, name="rt_envelope"))
con.sql("SELECT ST_Envelope(ST_GeogFromText('POINT (1
2)'))").to_arrow_table()
con.sql("SELECT rt_envelope(ST_GeogFromText('POINT (1
2)'))").to_arrow_table()
```
The direct call works, but the imported call reports `ST_Envelope() requires
a bounder for Spherical edges, which is not registered in this session`. The C
wrapper invokes the exported kernel with `None` for `ConfigOptions`, so it
cannot find the session's bounder. `ST_XMin` has the same issue; its previous
S2 kernel worked through this roundtrip.
--
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]
