Consolidate decimal error checking (apache#1438)

Can drop this after rebase on commit 4b454d0, first released in 7.0.0
cube-js · Aug 23, 2024 · 05cc54e · 05cc54e
1 parent 9e84e0f
commit 05cc54e
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 41 deletions.
diff --git a/datafusion/src/sql/planner.rs b/datafusion/src/sql/planner.rs
@@ -46,7 +46,7 @@ use crate::logical_plan::{
 };
 use crate::prelude::JoinType;
 use crate::scalar::ScalarValue;
-use crate::sql::utils::find_rolling_aggregate_exprs;
+use crate::sql::utils::{find_rolling_aggregate_exprs, make_decimal_type};
 use crate::{
     error::{DataFusionError, Result},
     physical_plan::udaf::AggregateUDF,
@@ -295,25 +295,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                 Ok(DataType::Utf8)
             }
             SQLDataType::Decimal(precision, scale) => {
-                match (precision, scale) {
-                    (None, _) | (_, None) => {
-                        return Err(DataFusionError::Internal(format!(
-                            "Invalid Decimal type ({:?}), precision or scale can't be empty.",
-                            sql_type
-                        )));
-                    }
-                    (Some(p), Some(s)) => {
-                        // TODO add bound checker in some utils file or function
-                        if *p > 38 || *s > *p {
-                            return Err(DataFusionError::Internal(format!(
-                                "Error Decimal Type ({:?}), precision must be less than or equal to 38 and scale can't be greater than precision",
-                                sql_type
-                            )));
-                        } else {
-                            Ok(DataType::Decimal(*p as usize, *s as usize))
-                        }
-                    }
-                }
+                make_decimal_type(*precision, *scale)
             }
             SQLDataType::Float(_) => Ok(DataType::Float32),
             SQLDataType::Real | SQLDataType::Double => Ok(DataType::Float64),
@@ -1957,27 +1939,7 @@ pub fn convert_data_type(sql_type: &SQLDataType) -> Result<DataType> {
         SQLDataType::Char(_) | SQLDataType::Varchar(_) => Ok(DataType::Utf8),
         SQLDataType::Timestamp => Ok(DataType::Timestamp(TimeUnit::Nanosecond, None)),
         SQLDataType::Date => Ok(DataType::Date32),
-        SQLDataType::Decimal(precision, scale) => {
-            match (precision, scale) {
-                (None, _) | (_, None) => {
-                    return Err(DataFusionError::Internal(format!(
-                        "Invalid Decimal type ({:?}), precision or scale can't be empty.",
-                        sql_type
-                    )));
-                }
-                (Some(p), Some(s)) => {
-                    // TODO add bound checker in some utils file or function
-                    if *p > 38 || *s > *p {
-                        return Err(DataFusionError::Internal(format!(
-                            "Error Decimal Type ({:?})",
-                            sql_type
-                        )));
-                    } else {
-                        Ok(DataType::Decimal(*p as usize, *s as usize))
-                    }
-                }
-            }
-        }
+        SQLDataType::Decimal(precision, scale) => make_decimal_type(*precision, *scale),
         other => Err(DataFusionError::NotImplemented(format!(
             "Unsupported SQL type {:?}",
             other

diff --git a/datafusion/src/sql/utils.rs b/datafusion/src/sql/utils.rs
@@ -17,6 +17,8 @@
 
 //! SQL Utility Functions
 
+use arrow::datatypes::DataType;
+
 use crate::logical_plan::{Expr, LogicalPlan};
 use crate::scalar::ScalarValue;
 use crate::{
@@ -527,6 +529,33 @@ pub(crate) fn group_window_expr_by_sort_keys(
     Ok(result)
 }
 
+/// Returns a validated `DataType` for the specified precision and
+/// scale
+pub(crate) fn make_decimal_type(
+    precision: Option<u64>,
+    scale: Option<u64>,
+) -> Result<DataType> {
+    match (precision, scale) {
+        (None, _) | (_, None) => {
+            return Err(DataFusionError::Internal(format!(
+                "Decimal(precision, scale) must both be specified, got ({:?}, {:?})",
+                precision, scale
+            )));
+        }
+        (Some(p), Some(s)) => {
+            // Arrow decimal is i128 meaning 38 maximum decimal digits
+            if p > 38 || s > p {
+                return Err(DataFusionError::Internal(format!(
+                    "For decimal(precision, scale) precision must be less than or equal to 38 and scale can't be greater than precision. Got ({}, {})",
+                    p, s
+                )));
+            } else {
+                Ok(DataType::Decimal(p as usize, s as usize))
+            }
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;