Skip to content

Commit

Permalink
Resolve unreachable branch of Value::position (gluesql#1012)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ever0de committed Nov 19, 2022
1 parent 2542a72 commit 4d3c3be
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 23 deletions.
4 changes: 2 additions & 2 deletions core/src/data/value/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
18 changes: 10 additions & 8 deletions core/src/data/value/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
}
Expand Down Expand Up @@ -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());
Expand All @@ -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())
);
Expand Down
13 changes: 5 additions & 8 deletions core/src/executor/evaluate/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>> {
Expand Down
2 changes: 1 addition & 1 deletion core/src/executor/evaluate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?;
Expand Down
2 changes: 1 addition & 1 deletion core/src/executor/evaluate/stateless.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down
13 changes: 10 additions & 3 deletions test-suite/src/function/position.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use {
crate::*,
gluesql_core::{
executor::EvaluateError,
prelude::{Payload, Value::*},
data::ValueError,
prelude::{
Payload,
Value::{self, *},
},
},
};

Expand All @@ -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 {
Expand Down

0 comments on commit 4d3c3be

Please sign in to comment.