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


##########
rust/sedona-raster-functions/src/rs_rastercoordinate.rs:
##########
@@ -139,17 +141,35 @@ impl SedonaScalarKernel for RsCoordinateMapper {
         let executor = RasterExecutor::new(arg_types, args);
         let mut builder = 
Int64Builder::with_capacity(executor.num_iterations());
 
-        let coord_opt = extract_scalar_coord(&args[1], &args[2])?;
-        executor.execute_raster_void(|_i, raster_opt| {
-            match (raster_opt, coord_opt) {
-                (Some(raster), (Some(x), Some(y))) => {
+        // Expand world x and y coordinate parameters to arrays and cast to 
Float64
+        let world_x_array = args[1].clone().cast_to(&DataType::Float64, None)?;
+        let world_x_array = 
world_x_array.into_array(executor.num_iterations())?;
+        let world_x_array = world_x_array.as_primitive::<Float64Type>();

Review Comment:
   Totally optional, but I have mostly been using DataFusion's helpers for 
these so far (`as_float64_array()`), which return errors in debug mode.



##########
rust/sedona-raster-functions/src/rs_rastercoordinate.rs:
##########
@@ -139,17 +141,35 @@ impl SedonaScalarKernel for RsCoordinateMapper {
         let executor = RasterExecutor::new(arg_types, args);
         let mut builder = 
Int64Builder::with_capacity(executor.num_iterations());
 
-        let coord_opt = extract_scalar_coord(&args[1], &args[2])?;
-        executor.execute_raster_void(|_i, raster_opt| {
-            match (raster_opt, coord_opt) {
-                (Some(raster), (Some(x), Some(y))) => {
+        // Expand world x and y coordinate parameters to arrays and cast to 
Float64
+        let world_x_array = args[1].clone().cast_to(&DataType::Float64, None)?;
+        let world_x_array = 
world_x_array.into_array(executor.num_iterations())?;
+        let world_x_array = world_x_array.as_primitive::<Float64Type>();
+        let world_y_array = args[2].clone().cast_to(&DataType::Float64, None)?;
+        let world_y_array = 
world_y_array.into_array(executor.num_iterations())?;
+        let world_y_array = world_y_array.as_primitive::<Float64Type>();
+
+        executor.execute_raster_void(|i, raster_opt| {
+            let x_opt = if world_x_array.is_null(i) {
+                None
+            } else {
+                Some(world_x_array.value(i))
+            };
+            let y_opt = if world_y_array.is_null(i) {
+                None
+            } else {
+                Some(world_y_array.value(i))
+            };

Review Comment:
   Also optional, but so far we have mostly implemented this by resolving the 
iterator before the loop and calling `x_opt.next().unwrap()`. In theory the 
iterator could be faster than calling `is_null()` and `value()` in a loop but 
probably isn't in practice.



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