Use new sedona_internal_err to avoid misleading datafusion internal err (#2)

* Add new sedona_internal_err

* Use sedona_internal_err instead of internal_err in st_xyzm.rs

* More replacements

* Use full import path in macro to avoid needing import

* Replace all uses in sedona-funcdtions

* Replace with 'sedona_internal_err' in remaining packages

* Update err test case in function_set.rs to use 'contains' instead of 'eq'

Author: petern48

Cargo.lock

@@ -0,0 +1,34 @@
+/// Macro to create Sedona Internal Error that avoids the misleading error message from
+/// DataFusionError::Internal.
+#[macro_export]
+macro_rules! sedona_internal_err {
+    ($($args:expr),*) => {{
+        let msg = std::format!(
+            "SedonaDB internal error: {}{}.\nThis issue was likely caused by a bug in SedonaDB's code. \
+            Please help us to resolve this by filing a bug report in our issue tracker: \
+            https://github.com/apache/sedona-db/issues",
+            std::format!($($args),*),
+            datafusion_common::DataFusionError::get_back_trace(),
+        );
+        // We avoid using Internal to avoid the message suggesting it's internal to DataFusion
+        Err(datafusion_common::DataFusionError::External(msg.into()))
+    }};
+}
+
+#[cfg(test)]
+mod tests {
+    use datafusion_common::DataFusionError;
+
+    #[test]
+    fn test_sedona_internal_err() {
+        let result: Result<(), DataFusionError> = sedona_internal_err!("Test error: {}", "details");
+        assert!(result.is_err());
+        let err = result.unwrap_err();
+        let err_string = err.to_string();
+        assert!(err_string.contains("SedonaDB internal error: Test error: details"));
+
+        // Ensure the message doesn't contain the 'DataFusion'-specific messages
+        assert!(!err_string.contains("DataFusion's code"));
+        assert!(!err_string.contains("https://github.com/apache/datafusion/issues"));
+    }
+}

rust/sedona-common/src/error.rs

@@ -3,6 +3,7 @@
 //! This crate contains shared components that are used across multiple
 //! Sedona modules, including configuration options and common data structures.
 
+pub mod error;
 pub mod option;
 
 pub use option::*;

rust/sedona-common/src/lib.rs

@@ -21,6 +21,7 @@ datafusion-common = { workspace = true }
 datafusion-expr = { workspace = true }
 datafusion-physical-expr = { workspace = true }
 geo-traits = { workspace = true }
+sedona-common = { path = "../sedona-common" }
 sedona-geometry = { path = "../sedona-geometry" }
 sedona-schema = { path = "../sedona-schema" }
 serde = { workspace = true }

rust/sedona-expr/Cargo.toml

@@ -2,8 +2,9 @@ use crate::{
     aggregate_udf::{SedonaAccumulatorRef, SedonaAggregateUDF},
     scalar_udf::{ScalarKernelRef, SedonaScalarUDF},
 };
-use datafusion_common::{error::Result, internal_err};
+use datafusion_common::error::Result;
 use datafusion_expr::{AggregateUDFImpl, ScalarUDFImpl};
+use sedona_common::sedona_internal_err;
 use std::collections::HashMap;
 
 /// Helper for managing groups of functions
@@ -110,7 +111,7 @@ impl FunctionSet {
             function.add_kernel(kernel);
             Ok(self.aggregate_udf(name).unwrap())
         } else {
-            internal_err!("Can't register aggregate kernel for function '{}'", name)
+            sedona_internal_err!("Can't register aggregate kernel for function '{}'", name)
         }
     }
 }
@@ -248,10 +249,9 @@ mod tests {
         let err = functions
             .add_aggregate_udf_kernel("function that does not exist", kernel.clone())
             .unwrap_err();
-        assert_eq!(
-            err.message().lines().next().unwrap(),
+        assert!(err.message().lines().next().unwrap().contains(
             "Can't register aggregate kernel for function 'function that does not exist'."
-        );
+        ));
 
         let udaf2 = SedonaAggregateUDF::new(
             "simple_udaf2",

rust/sedona-expr/src/function_set.rs

@@ -3,11 +3,12 @@ use std::sync::Arc;
 use std::{any::Any, fmt::Debug};
 
 use arrow_schema::{DataType, Field, FieldRef};
-use datafusion_common::{internal_err, not_impl_err, plan_err, Result, ScalarValue};
+use datafusion_common::{not_impl_err, plan_err, Result, ScalarValue};
 use datafusion_expr::{
     ColumnarValue, Documentation, ReturnFieldArgs, ScalarFunctionArgs, ScalarUDFImpl, Signature,
     Volatility,
 };
+use sedona_common::sedona_internal_err;
 use sedona_schema::datatypes::{Edges, SedonaType};
 
 pub type ScalarKernelRef = Arc<dyn SedonaScalarKernel + Send + Sync>;
@@ -121,7 +122,7 @@ impl ArgMatcher {
                     (Some(lhs_crs), Some(rhs_crs)) => {
                         plan_err!("Mismatched CRS arguments: {lhs_crs} vs {rhs_crs}\n{hint}")
                     }
-                    _ => internal_err!("None vs. None should be considered equal"),
+                    _ => sedona_internal_err!("None vs. None should be considered equal"),
                 };
             }
         }
@@ -876,13 +877,13 @@ mod tests {
                 }
             }
 
-            internal_err!("unrecognized target value")
+            sedona_internal_err!("unrecognized target value")
         }
     }
 
     impl SedonaScalarKernel for SimpleCast {
         fn return_type(&self, _args: &[SedonaType]) -> Result<Option<SedonaType>> {
-            internal_err!("Should not be called")
+            sedona_internal_err!("Should not be called")
         }
 
         fn return_type_from_args_and_scalars(

rust/sedona-expr/src/scalar_udf.rs

@@ -1,12 +1,13 @@
 use std::sync::Arc;
 
-use datafusion_common::{internal_err, DataFusionError, Result, ScalarValue};
+use datafusion_common::{DataFusionError, Result, ScalarValue};
 use datafusion_expr::Operator;
 use datafusion_physical_expr::{
     expressions::{BinaryExpr, Column, Literal},
     PhysicalExpr, ScalarFunctionExpr,
 };
 use geo_traits::Dimensions;
+use sedona_common::sedona_internal_err;
 use sedona_geometry::{bounding_box::BoundingBox, bounds::wkb_bounds_xy, interval::IntervalTrait};
 use sedona_schema::datatypes::SedonaType;
 
@@ -107,7 +108,9 @@ impl SpatialFilter {
             match scalar_fun.fun().name() {
                 "st_intersects" => {
                     if args.len() != 2 {
-                        return internal_err!("unexpected argument count in filter evaluation");
+                        return sedona_internal_err!(
+                            "unexpected argument count in filter evaluation"
+                        );
                     }
 
                     match (&args[0], &args[1]) {
@@ -126,7 +129,9 @@ impl SpatialFilter {
                 }
                 "st_hasz" => {
                     if args.len() != 1 {
-                        return internal_err!("unexpected argument count in filter evaluation");
+                        return sedona_internal_err!(
+                            "unexpected argument count in filter evaluation"
+                        );
                     }
 
                     match &args[0] {
@@ -191,7 +196,7 @@ fn literal_bounds(literal: &Literal) -> Result<BoundingBox> {
         _ => {}
     }
 
-    internal_err!("Unexpected scalar type in filter expression")
+    sedona_internal_err!("Unexpected scalar type in filter expression")
 }
 
 fn parse_args(args: &[Arc<dyn PhysicalExpr>]) -> Vec<ArgRef<'_>> {

rust/sedona-expr/src/spatial_filter.rs

@@ -25,6 +25,7 @@ arrow-array = { workspace = true }
 datafusion-common = { workspace = true }
 datafusion-expr = { workspace = true }
 geo-traits = { workspace = true }
+sedona-common = { path = "../sedona-common" }
 sedona-expr = { path = "../sedona-expr" }
 sedona-geometry = { path = "../sedona-geometry" }
 sedona-schema = { path = "../sedona-schema" }

rust/sedona-functions/Cargo.toml

@@ -3,8 +3,9 @@ use std::iter::zip;
 use arrow_array::ArrayRef;
 use datafusion_common::cast::{as_binary_array, as_binary_view_array};
 use datafusion_common::error::Result;
-use datafusion_common::{internal_err, DataFusionError, ScalarValue};
+use datafusion_common::{DataFusionError, ScalarValue};
 use datafusion_expr::ColumnarValue;
+use sedona_common::sedona_internal_err;
 use sedona_schema::datatypes::SedonaType;
 use wkb::reader::Wkb;
 
@@ -300,7 +301,7 @@ impl IterGeo for ArrayRef {
         func: F,
     ) -> Result<()> {
         if num_iterations != self.len() {
-            return internal_err!(
+            return sedona_internal_err!(
                 "Expected {num_iterations} items but got Array with {} items",
                 self.len()
             );
@@ -313,7 +314,7 @@ impl IterGeo for ArrayRef {
                 // We could cast here as a fallback, iterate and cast per-element, or
                 // implement iter_as_something_else()/supports_iter_xxx() when more geo array types
                 // are supported.
-                internal_err!("Can't iterate over {:?} as Wkb", sedona_type)
+                sedona_internal_err!("Can't iterate over {:?} as Wkb", sedona_type)
             }
         }
     }
@@ -326,7 +327,7 @@ impl ScalarGeo for ScalarValue {
             | ScalarValue::BinaryView(maybe_item)
             | ScalarValue::LargeBinary(maybe_item) => Ok(maybe_item.as_deref()),
             ScalarValue::Null => Ok(None),
-            _ => internal_err!("Can't iterate over {:?} ScalarValue as &[u8]", self),
+            _ => sedona_internal_err!("Can't iterate over {:?} ScalarValue as &[u8]", self),
         }
     }
 }
@@ -379,7 +380,7 @@ fn iter_wkb_wkb_array<
         _ => {
             // We could do casting of one or both sides to support other cases as they
             // arise to manage the complexity/performance balance
-            internal_err!(
+            sedona_internal_err!(
                 "Can't iterate over {:?} and {:?} arrays as a pair of Wkb scalars",
                 types.0,
                 types.1

rust/sedona-functions/src/executor.rs

@@ -3,11 +3,12 @@ use std::sync::Arc;
 use crate::executor::WkbExecutor;
 use arrow_array::builder::Int8Builder;
 use arrow_schema::DataType;
-use datafusion_common::{error::Result, internal_err};
+use datafusion_common::error::Result;
 use datafusion_expr::{
     scalar_doc_sections::DOC_SECTION_OTHER, ColumnarValue, Documentation, Volatility,
 };
 use geo_traits::{GeometryCollectionTrait, GeometryTrait, GeometryType};
+use sedona_common::sedona_internal_err;
 use sedona_expr::scalar_udf::{ArgMatcher, SedonaScalarKernel, SedonaScalarUDF};
 use sedona_schema::datatypes::SedonaType;
 use wkb::reader::Wkb;
@@ -76,7 +77,7 @@ fn invoke_scalar(item: &Wkb) -> Result<i8> {
             }
             Ok(highest_dim)
         }
-        _ => internal_err!("Invalid geometry type"),
+        _ => sedona_internal_err!("Invalid geometry type"),
     }
 }