Resolve unreachable branch of Value::position (#1012)

The unit test coverage is satisfied, but the error branch that did not appear in the user interface Glue::execute is corrected. If it were not for this fix, the position function would probably be correct to receive a String rather than a Value.
gluesql · Nov 19, 2022 · 4d3c3be · 4d3c3be
1 parent 2542a72
commit 4d3c3be
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 23 deletions.
diff --git a/core/src/data/value/error.rs b/core/src/data/value/error.rs
@@ -177,8 +177,8 @@ pub enum ValueError {
     #[error("non numeric value in sqrt {0:?}")]
     SqrtOnNonNumeric(Value),
 
-    #[error("unsupported value by position function: from_str(from_str:?), sub_str(sub_str:?)")]
-    UnSupportedValueByPositionFunction { from_str: Value, sub_str: Value },
+    #[error("non-string parameter in position: {} IN {}", String::from(.from), String::from(.sub))]
+    NonStringParameterInPosition { from: Value, sub: Value },
 
     #[error("failed to convert Value to Expr")]
     ValueToExprConversionFailure,

diff --git a/core/src/data/value/mod.rs b/core/src/data/value/mod.rs
@@ -607,20 +607,21 @@ impl Value {
     ///
     /// let str1 = Value::Str("ramen".to_owned());
     /// let str2 = Value::Str("men".to_owned());
+    ///
     /// assert_eq!(str1.position(&str2), Ok(Value::I64(3)));
     /// assert_eq!(str2.position(&str1), Ok(Value::I64(0)));
     /// assert!(Value::Null.position(&str2).unwrap().is_null());
     /// assert!(str1.position(&Value::Null).unwrap().is_null());
     /// ```
-
     pub fn position(&self, other: &Value) -> Result<Value> {
         use Value::*;
+
         match (self, other) {
-            (Str(from_str), Str(sub_str)) => Ok(I64(str_position(from_str, sub_str) as i64)),
+            (Str(from), Str(sub)) => Ok(I64(str_position(from, sub) as i64)),
             (Null, _) | (_, Null) => Ok(Null),
-            _ => Err(ValueError::UnSupportedValueByPositionFunction {
-                from_str: self.clone(),
-                sub_str: other.clone(),
+            _ => Err(ValueError::NonStringParameterInPosition {
+                from: self.clone(),
+                sub: other.clone(),
             }
             .into()),
         }
@@ -1660,6 +1661,7 @@ mod tests {
         let str1 = Str("ramen".to_owned());
         let str2 = Str("men".to_owned());
         let empty_str = Str("".to_owned());
+
         assert_eq!(str1.position(&str2), Ok(I64(3)));
         assert_eq!(str2.position(&str1), Ok(I64(0)));
         assert!(Null.position(&str2).unwrap().is_null());
@@ -1668,9 +1670,9 @@ mod tests {
         assert_eq!(str1.position(&empty_str), Ok(I64(0)));
         assert_eq!(
             str1.position(&I64(1)),
-            Err(ValueError::UnSupportedValueByPositionFunction {
-                from_str: str1,
-                sub_str: I64(1)
+            Err(ValueError::NonStringParameterInPosition {
+                from: str1,
+                sub: I64(1)
             }
             .into())
         );

diff --git a/core/src/executor/evaluate/function.rs b/core/src/executor/evaluate/function.rs
@@ -584,14 +584,11 @@ pub fn to_time<'a>(
     }
 }
 
-pub fn position<'a>(
-    name: String,
-    from_expr: Evaluated<'_>,
-    sub_expr: Evaluated<'_>,
-) -> Result<Evaluated<'a>> {
-    let from_expr = eval_to_str!(name, from_expr);
-    let sub_expr = eval_to_str!(name, sub_expr);
-    Value::position(&Value::Str(from_expr), &Value::Str(sub_expr)).map(Evaluated::from)
+pub fn position<'a>(from_expr: Evaluated<'_>, sub_expr: Evaluated<'_>) -> Result<Evaluated<'a>> {
+    let from: Value = from_expr.try_into()?;
+    let sub: Value = sub_expr.try_into()?;
+
+    from.position(&sub).map(Evaluated::from)
 }
 
 pub fn cast<'a>(expr: Evaluated<'a>, data_type: &DataType) -> Result<Evaluated<'a>> {

diff --git a/core/src/executor/evaluate/mod.rs b/core/src/executor/evaluate/mod.rs
@@ -446,7 +446,7 @@ async fn evaluate_function<'a>(
         } => {
             let from_expr = eval(from_expr).await?;
             let sub_expr = eval(sub_expr).await?;
-            f::position(name, from_expr, sub_expr)
+            f::position(from_expr, sub_expr)
         }
         Function::Cast { expr, data_type } => {
             let expr = eval(expr).await?;

diff --git a/core/src/executor/evaluate/stateless.rs b/core/src/executor/evaluate/stateless.rs
@@ -329,7 +329,7 @@ fn evaluate_function<'a>(
         } => {
             let from_expr = eval(from_expr)?;
             let sub_expr = eval(sub_expr)?;
-            f::position(name, from_expr, sub_expr)
+            f::position(from_expr, sub_expr)
         }
         Function::Cast { expr, data_type } => {
             let expr = eval(expr)?;

diff --git a/test-suite/src/function/position.rs b/test-suite/src/function/position.rs
@@ -1,8 +1,11 @@
 use {
     crate::*,
     gluesql_core::{
-        executor::EvaluateError,
-        prelude::{Payload, Value::*},
+        data::ValueError,
+        prelude::{
+            Payload,
+            Value::{self, *},
+        },
     },
 };
 
@@ -25,7 +28,11 @@ test_case!(position, async move {
         ),
         (
             "SELECT POSITION(1 IN 'cheese') AS test",
-            Err(EvaluateError::FunctionRequiresStringValue(String::from("POSITION")).into()),
+            Err(ValueError::NonStringParameterInPosition {
+                from: Value::Str("cheese".to_owned()),
+                sub: Value::I64(1),
+            }
+            .into()),
         ),
     ];
     for (sql, expected) in test_cases {