paleolimbot commented on code in PR #38:
URL: https://github.com/apache/sedona-db/pull/38#discussion_r2337606687


##########
rust/sedona-functions/src/st_srid.rs:
##########
@@ -79,16 +103,58 @@ impl SedonaScalarKernel for StSrid {
             _ => Some(0),
         };
 
-        executor.execute_wkb_void(|maybe_wkb| {
-            match maybe_wkb {
-                Some(_wkb) => {
-                    builder.append_option(srid_opt);
-                }
-                _ => builder.append_null(),
+        if let ColumnarValue::Array(array) = &args[0] {
+            (0..array.len()).for_each(|i| {
+                builder.append_option(if array.is_null(i) { None } else { 
srid_opt });
+            });
+        } else if let ColumnarValue::Scalar(scalar) = &args[0] {
+            builder.append_option(if scalar.is_null() { None } else { srid_opt 
});
+        }

Review Comment:
   This works too!
   
   I usually see this written as
   
   ```rust
   match &args[0] {
     ColumnarValue::Array(array) => {...}
     ColumnarValue::Scalar(scalar) => {...}
   }
   ```
   
   ...but either is ok!



##########
python/sedonadb/tests/functions/test_transforms.py:
##########
@@ -64,3 +80,15 @@ def test_st_setcrs_sedonadb(eng, geom, crs, expected_srid):
     result = eng.execute_and_collect(f"SELECT ST_SetCrs({geom_or_null(geom)}, 
'{crs}')")
     df = eng.result_to_pandas(result)
     assert df.crs.to_epsg() == expected_srid
+
[email protected]("eng", [SedonaDB])
+def test_st_crs_sedonadb(eng):
+    eng = eng.create_or_skip()
+    eng.assert_query_result(
+        f"SELECT ST_CRS(ST_SetCrs(ST_GeomFromText('POINT (1 1)'), 
'EPSG:26920'))",
+        "\"EPSG:26920\""
+    )
+    eng.assert_query_result(
+        f"SELECT ST_CRS(ST_SetCrs(ST_GeomFromText('POINT (1 1)'), NULL))",
+        None,
+    )

Review Comment:
   `pre-commit run --all-files` may complain about the end of this file 
(because it lacks a newline)



##########
rust/sedona-functions/src/st_srid.rs:
##########
@@ -152,4 +222,39 @@ mod test {
         assert!(result.is_err());
         assert!(result.unwrap_err().to_string().contains("CRS has no SRID"));
     }
+
+    #[test]
+    fn udf_crs() {
+        let udf: ScalarUDF = st_crs_udf().into();
+
+        // Test that when no CRS is set, CRS is null
+        let sedona_type = SedonaType::Wkb(Edges::Planar, None);
+        let tester = ScalarUdfTester::new(udf.clone(), vec![sedona_type]);
+        tester.assert_return_type(DataType::Utf8View);
+        let result = tester
+            .invoke_scalar("POLYGON ((0 0, 1 0, 0 1, 0 0))")
+            .unwrap();
+        tester.assert_scalar_result_equals(result, ScalarValue::Utf8(None));
+
+        // Test that NULL input returns NULL output
+        let result = tester.invoke_scalar(ScalarValue::Null).unwrap();
+        tester.assert_scalar_result_equals(result, ScalarValue::Null);
+
+        // Test with a CRS with an EPSG code
+        let crs_value = serde_json::Value::String("EPSG:4837".to_string());
+        let crs = deserialize_crs(&crs_value).unwrap();
+        let sedona_type = SedonaType::Wkb(Edges::Planar, crs.clone());
+        let tester = ScalarUdfTester::new(udf.clone(), vec![sedona_type]);
+        let result = tester
+            .invoke_scalar("POLYGON ((0 0, 1 0, 0 1, 0 0))")
+            .unwrap();
+        tester.assert_scalar_result_equals(
+            result,
+            ScalarValue::Utf8(Some("\"EPSG:4837\"".to_string())),
+        );
+
+        // Test with a CRS but null geom
+        let result = tester.invoke_scalar(ScalarValue::Null).unwrap();
+        tester.assert_scalar_result_equals(result, ScalarValue::Null);

Review Comment:
   This (and the SRID test) would benefit from a test that uses 
`tester.invoke_array(create_array(&[Some(...), None]))` since that's now 
special-cased in the implementation.



-- 
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]

Reply via email to