paleolimbot commented on code in PR #275:
URL: https://github.com/apache/sedona-db/pull/275#discussion_r2506263718
##########
rust/sedona-functions/src/st_setsrid.rs:
##########
@@ -227,6 +227,89 @@ fn determine_return_type(
sedona_internal_err!("Unexpected argument types: {}, {}", args[0], args[1])
}
+/// [SedonaScalarKernel] wrapper that handles the SRID argument for
constructors like ST_Point
+#[derive(Debug)]
+pub(crate) struct SRIDifiedKernel {
+ inner: ScalarKernelRef,
+}
+
+impl SRIDifiedKernel {
+ pub(crate) fn new(inner: ScalarKernelRef) -> Self {
+ Self { inner }
+ }
+}
+
+impl SedonaScalarKernel for SRIDifiedKernel {
+ fn return_type_from_args_and_scalars(
+ &self,
+ args: &[SedonaType],
+ scalar_args: &[Option<&ScalarValue>],
+ ) -> Result<Option<SedonaType>> {
+ // args should consist of the original args and one extra arg for
+ // specifying CRS. So, first, validate the length and separate these.
+ //
+ // [arg0, arg1, ..., crs_arg];
+ // ^^^^^^^^^^^^^^^
+ // orig_args
+ let orig_args_len = match (args.len(), scalar_args.len()) {
+ (0, 0) => return Ok(None),
+ (l1, l2) if l1 == l2 => l1 - 1,
+ _ => return sedona_internal_err!("Arg types and arg values have
different lengths"),
+ };
+
+ let orig_args = &args[..orig_args_len];
+ let orig_scalar_args = &scalar_args[..orig_args_len];
+
+ let crs = match scalar_args[orig_args_len] {
+ Some(crs) => crs,
+ None => return Ok(None),
+ };
+ let new_crs = match crs.cast_to(&DataType::Utf8) {
+ Ok(ScalarValue::Utf8(Some(crs))) => {
+ if crs == "0" {
+ None
+ } else {
+ validate_crs(&crs, None)?;
+ deserialize_crs(&serde_json::Value::String(crs))?
+ }
+ }
+ Ok(ScalarValue::Utf8(None)) => None,
+ Ok(_) | Err(_) => return sedona_internal_err!("Can't cast Crs
{crs:?} to Utf8"),
+ };
+
+ let mut result = self
+ .inner
+ .return_type_from_args_and_scalars(orig_args, orig_scalar_args);
+ if let Ok(Some(sedona_type)) = &mut result {
+ match sedona_type {
+ SedonaType::Wkb(_, crs) => *crs = new_crs,
+ SedonaType::WkbView(_, crs) => *crs = new_crs,
+ _ => {
+ return sedona_internal_err!("Return type must be Wkb or
WkbView");
+ }
+ }
+ }
+
+ result
+ }
+
+ fn invoke_batch(
+ &self,
+ arg_types: &[SedonaType],
+ args: &[ColumnarValue],
+ ) -> Result<ColumnarValue> {
+ let orig_args_len = arg_types.len() - 1;
+ self.inner
+ .invoke_batch(&arg_types[..orig_args_len], &args[..orig_args_len])
+ }
Review Comment:
I think this is the place where we'd have to check `if let
ColumnarValue::Scalar(sc) = args[orig_args_len] { if sc.is_null()`, and perhaps
modify the validity buffer of the `inner.invoke_batch().to_array()`. Feel free
to punt on that and file a follow-on ticket 🙂
##########
python/sedonadb/tests/functions/test_functions.py:
##########
@@ -1313,6 +1313,31 @@ def test_st_point(eng, x, y, expected):
)
[email protected]("eng", [SedonaDB, PostGIS])
[email protected](
+ ("x", "y", "srid", "expected"),
+ [
+ (None, None, None, None),
+ # TODO: This is a bit tricky, but in PostGIS, NULL and unknown CRS are
distinguished.
+ #
+ # - ST_SRID(ST_POINT(x, y, NULL)) returns NULL
+ # - ST_SRID(ST_POINT(x, y, 0)) returns 0
+ # - ST_SRID(ST_POINT(x, y)) returns 0
+ #
+ # (1, 1, None, None),
Review Comment:
I think that `ST_SetSRID()` handles this in the same way as PostGIS and it
would be helpful to handle that here (I'll leave a suggestion below about how
we might do that)
--
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]
