Repository: gluesql/gluesql
Pull request #162 (2021-03-05 15:04:35)

Change literal casting function to return from AstValue to Value,
Add `Value::TryFromLiteral` to replace `cast_ast_value` in `evaluated.rs`.
Now it can handle `CAST(1 AS FLOAT)`.
Test cases in `function::cast::cast_literal` reach all branches in `Value::TryFromLiteral` match.

@KyGost You even left a comment about `CAST(1 AS FLOAT)` issue but I missed to check it.
I just figured out and that was because of... function return value type.
Previous: `AstValue -> AstValue`
Current: `AstValue -> Value`
This was possible because `CAST` provides `DataType` information.

And I modified `cast_literal` test cases little bit to reach every branches in `match`
```rust
impl TryFromLiteral for Value {
    fn try_from_literal(data_type: &DataType, literal: &AstValue) -> Result<Value> {
       match ...
    }
}
```
Order of match branches (`try_from_literal`) and test cases (`function::cast::cast_literal`) is same, you will be able to check simply following the code lines.
`CAST` were too big task so my review was.... not sufficient, sorry for that. Writing test cases to reach every branches would be helpful for you to work on https://github.com/gluesql/gluesql/pull/157

I also found some issues on handling `cast_value`, I might work for casting value on Sunday, then I'll also share with you. :)


KyGost: Oh, we've gone back to always converting `AstValue` to `Value` I thought we were avoiding this?

I figured we'd be able to fix the float issue with natural conversion.
panarch: @KyGost
`AstValue` -> `Value` is good and it's right direction because our execute's return data is `Vec<Value>`.
`Value` -> `AstValue` is the thing that we need to avoid, it can make cycle in `Evaluated`.

Though `Evaluated` has 5 types, basically you can simplify into two, `Literal (AstValue)` and `Value`.

* `Literal` -> `Literal` or `Value`
* `Value` -> `Value`

It's all :)

https://github.com/gluesql/gluesql/blob/d642c75a84267f1a60e4505119ca6fdcde516e1f/src/data/value/mod.rs#L132
```rust
pub fn from_data_type(
        data_type: &DataType,
        nullable: bool,
        literal: &AstValue,
    ) -> Result<Self> {
```
Here, the condition to make `Value` requires three params, data type, nullable and literal.
It's little bit different from `TryFromLiteral` because it only takes two, there's no nullable.
However, it is ok for `TryFromLiteral` not to use nullable.
This is due to...
1. `CAST` cannot provide that.
2. All compare & calculation methods in `Value` (`PartialEq`, `PartialOrd`, `add`, `subtract`.. ) supports operation between non-nullable enums and `Opt-` prefixed nullable enums.

e.g.
```
Value::I64(100) + Value::OptI64(Some(200)) => Value::OptI64(Some(300))
```



Patch:
diff --git a/src/data/mod.rs b/src/data/mod.rs
index 5a3df496..e29de421 100644
--- a/src/data/mod.rs
+++ b/src/data/mod.rs
@@ -1,7 +1,7 @@
 mod row;
 mod schema;
 mod table;
-mod value;
+pub mod value;
 
 pub use {
     row::{Row, RowError},
diff --git a/src/data/value/ast_value.rs b/src/data/value/ast_value.rs
index 1562262d..1c508bbe 100644
--- a/src/data/value/ast_value.rs
+++ b/src/data/value/ast_value.rs
@@ -1,7 +1,7 @@
 use {
     super::{error::ValueError, Value},
     crate::result::{Error, Result},
-    sqlparser::ast::Value as AstValue,
+    sqlparser::ast::{DataType, Value as AstValue},
     std::{cmp::Ordering, convert::TryFrom},
 };
 
@@ -78,3 +78,60 @@ impl TryFrom<&AstValue> for Value {
         }
     }
 }
+
+pub trait TryFromLiteral {
+    fn try_from_literal(data_type: &DataType, literal: &AstValue) -> Result<Value>;
+}
+
+impl TryFromLiteral for Value {
+    fn try_from_literal(data_type: &DataType, literal: &AstValue) -> Result<Value> {
+        match (data_type, literal) {
+            (DataType::Boolean, AstValue::SingleQuotedString(v))
+            | (DataType::Boolean, AstValue::Number(v, false)) => match v.to_uppercase().as_str() {
+                "TRUE" | "1" => Ok(Value::Bool(true)),
+                "FALSE" | "0" => Ok(Value::Bool(false)),
+                _ => Err(ValueError::LiteralCastToBooleanFailed(v.to_string()).into()),
+            },
+            (DataType::Boolean, AstValue::Null) => Ok(Value::OptBool(None)),
+            (DataType::Int, AstValue::SingleQuotedString(v)) => v
+                .parse::<i64>()
+                .map(Value::I64)
+                .map_err(|_| ValueError::LiteralCastFromTextToIntegerFailed(v.to_string()).into()),
+            (DataType::Int, AstValue::Number(v, false)) => v
+                .parse::<f64>()
+                .map_err(|_| {
+                    ValueError::UnreachableLiteralCastFromNumberToInteger(v.to_string()).into()
+                })
+                .map(|v| Value::I64(v.trunc() as i64)),
+            (DataType::Int, AstValue::Boolean(v)) => {
+                let v = if *v { 1 } else { 0 };
+
+                Ok(Value::I64(v))
+            }
+            (DataType::Int, AstValue::Null) => Ok(Value::OptI64(None)),
+            (DataType::Float(_), AstValue::SingleQuotedString(v))
+            | (DataType::Float(_), AstValue::Number(v, false)) => v
+                .parse::<f64>()
+                .map(Value::F64)
+                .map_err(|_| ValueError::LiteralCastToFloatFailed(v.to_string()).into()),
+            (DataType::Float(_), AstValue::Boolean(v)) => {
+                let v = if *v { 1.0 } else { 0.0 };
+
+                Ok(Value::F64(v))
+            }
+            (DataType::Float(_), AstValue::Null) => Ok(Value::OptF64(None)),
+            (DataType::Text, AstValue::Number(v, false)) => Ok(Value::Str(v.to_string())),
+            (DataType::Text, AstValue::Boolean(v)) => {
+                let v = if *v { "TRUE" } else { "FALSE" };
+
+                Ok(Value::Str(v.to_owned()))
+            }
+            (DataType::Text, AstValue::Null) => Ok(Value::OptStr(None)),
+            _ => Err(ValueError::UnimplementedLiteralCast {
+                data_type: data_type.to_string(),
+                literal: literal.to_string(),
+            }
+            .into()),
+        }
+    }
+}
diff --git a/src/data/value/error.rs b/src/data/value/error.rs
index 84235690..f3001776 100644
--- a/src/data/value/error.rs
+++ b/src/data/value/error.rs
@@ -41,9 +41,26 @@ pub enum ValueError {
     #[error("floating columns cannot be set to unique constraint")]
     ConflictOnFloatWithUniqueConstraint,
 
+    // Cast errors from value to value
     #[error("impossible cast")]
     ImpossibleCast,
 
     #[error("unimplemented cast")]
     UnimplementedCast,
+
+    // Cast errors from literal to value
+    #[error("literal cast failed from text to integer: {0}")]
+    LiteralCastFromTextToIntegerFailed(String),
+
+    #[error("literal cast failed from text to float: {0}")]
+    LiteralCastToFloatFailed(String),
+
+    #[error("literal cast failed to boolean: {0}")]
+    LiteralCastToBooleanFailed(String),
+
+    #[error("unreachable literal cast from number to integer: {0}")]
+    UnreachableLiteralCastFromNumberToInteger(String),
+
+    #[error("unimplemented literal cast: {literal} as {data_type}")]
+    UnimplementedLiteralCast { data_type: String, literal: String },
 }
diff --git a/src/data/value/mod.rs b/src/data/value/mod.rs
index f26d70a7..74834705 100644
--- a/src/data/value/mod.rs
+++ b/src/data/value/mod.rs
@@ -11,6 +11,7 @@ mod error;
 mod group_key;
 mod unique_key;
 
+pub use ast_value::TryFromLiteral;
 pub use error::ValueError;
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
diff --git a/src/executor/evaluate/error.rs b/src/executor/evaluate/error.rs
index f85962b0..c56783a3 100644
--- a/src/executor/evaluate/error.rs
+++ b/src/executor/evaluate/error.rs
@@ -45,15 +45,6 @@ pub enum EvaluateError {
     #[error("unimplemented")]
     Unimplemented,
 
-    #[error("impossible cast")]
-    ImpossibleCast,
-
-    #[error("unimplemented cast")]
-    UnimplementedCast,
-
-    #[error("unreachable impossible cast")]
-    UnreachableImpossibleCast,
-
     #[error("unreachable named function argument: {0}")]
     UnreachableFunctionArg(String),
 }
diff --git a/src/executor/evaluate/evaluated.rs b/src/executor/evaluate/evaluated.rs
index ad5e2f5b..6fc1ebf6 100644
--- a/src/executor/evaluate/evaluated.rs
+++ b/src/executor/evaluate/evaluated.rs
@@ -2,7 +2,7 @@ use {
     super::EvaluateError,
     crate::{
         data,
-        data::Value,
+        data::value::{TryFromLiteral, Value},
         result::{Error, Result},
     },
     sqlparser::ast::{DataType, Value as AstValue},
@@ -230,9 +230,8 @@ impl<'a> Evaluated<'a> {
     unary_op!(unary_minus, -);
 
     pub fn cast(self, data_type: &DataType) -> Result<Evaluated<'a>> {
-        let cast_literal =
-            |value: &AstValue| cast_ast_value(value, data_type).map(Evaluated::Literal);
-        let cast_value = |value: &data::Value| value.cast(data_type).map(Evaluated::Value);
+        let cast_literal = |literal: &AstValue| Value::try_from_literal(data_type, literal);
+        let cast_value = |value: &data::Value| value.cast(data_type);
 
         match self {
             StringRef(value) => cast_literal(&AstValue::SingleQuotedString(value.to_string())),
@@ -241,6 +240,7 @@ impl<'a> Evaluated<'a> {
             ValueRef(value) => cast_value(value),
             Value(value) => cast_value(&value),
         }
+        .map(Evaluated::Value)
     }
 
     pub fn is_some(&self) -> bool {
@@ -253,44 +253,3 @@ impl<'a> Evaluated<'a> {
         }
     }
 }
-
-fn cast_ast_value(value: &AstValue, data_type: &DataType) -> Result<AstValue> {
-    match (data_type, value) {
-        (DataType::Boolean, AstValue::SingleQuotedString(value))
-        | (DataType::Boolean, AstValue::Number(value, false)) => {
-            Ok(match value.to_uppercase().as_str() {
-                "TRUE" | "1" => Ok(AstValue::Boolean(true)),
-                "FALSE" | "0" => Ok(AstValue::Boolean(false)),
-                _ => Err(EvaluateError::ImpossibleCast),
-            }?)
-        }
-        (DataType::Int, AstValue::Number(value, false)) => Ok(AstValue::Number(
-            value
-                .parse::<f64>()
-                .map_err(|_| EvaluateError::UnreachableImpossibleCast)?
-                .trunc()
-                .to_string(),
-            false,
-        )),
-        (DataType::Int, AstValue::SingleQuotedString(value))
-        | (DataType::Float(_), AstValue::SingleQuotedString(value)) => {
-            Ok(AstValue::Number(value.to_string(), false))
-        }
-        (DataType::Int, AstValue::Boolean(value))
-        | (DataType::Float(_), AstValue::Boolean(value)) => Ok(AstValue::Number(
-            (if *value { "1" } else { "0" }).to_string(),
-            false,
-        )),
-        (DataType::Float(_), AstValue::Number(value, false)) => {
-            Ok(AstValue::Number(value.to_string(), false))
-        }
-        (DataType::Text, AstValue::Boolean(value)) => Ok(AstValue::SingleQuotedString(
-            (if *value { "TRUE" } else { "FALSE" }).to_string(),
-        )),
-        (DataType::Text, AstValue::Number(value, false)) => {
-            Ok(AstValue::SingleQuotedString(value.to_string()))
-        }
-        (_, AstValue::Null) => Ok(AstValue::Null),
-        _ => Err(EvaluateError::UnimplementedCast.into()),
-    }
-}


Test patch:
diff --git a/src/tests/function/cast.rs b/src/tests/function/cast.rs
index 4b3e4209..12254806 100644
--- a/src/tests/function/cast.rs
+++ b/src/tests/function/cast.rs
@@ -1,107 +1,118 @@
 use crate::*;
 
-test_case!(cast, async move {
-    use Value::{Bool, Str, F64, I64};
+test_case!(cast_literal, async move {
+    use Value::*;
+
     let test_cases = vec![
         ("CREATE TABLE Item (number TEXT)", Ok(Payload::Create)),
+        (r#"INSERT INTO Item VALUES ("1")"#, Ok(Payload::Insert(1))),
+        (
+            r#"SELECT CAST("TRUE" AS BOOLEAN) AS cast FROM Item"#,
+            Ok(select!(cast Bool; true)),
+        ),
+        (
+            r#"SELECT CAST(1 AS BOOLEAN) AS cast FROM Item"#,
+            Ok(select!(cast Bool; true)),
+        ),
+        (
+            r#"SELECT CAST("asdf" AS BOOLEAN) AS cast FROM Item"#,
+            Err(ValueError::LiteralCastToBooleanFailed("asdf".to_owned()).into()),
+        ),
+        (
+            r#"SELECT CAST(3 AS BOOLEAN) AS cast FROM Item"#,
+            Err(ValueError::LiteralCastToBooleanFailed("3".to_owned()).into()),
+        ),
+        (
+            r#"SELECT CAST(NULL AS BOOLEAN) AS cast FROM Item"#,
+            Ok(select!(cast OptBool; None)),
+        ),
+        (
+            r#"SELECT CAST("1" AS INTEGER) AS cast FROM Item"#,
+            Ok(select!(cast I64; 1)),
+        ),
         (
-            r#"INSERT INTO Item VALUES ("1"), ("2"), ("3")"#,
-            Ok(Payload::Insert(3)),
+            r#"SELECT CAST("foo" AS INTEGER) AS cast FROM Item"#,
+            Err(ValueError::LiteralCastFromTextToIntegerFailed("foo".to_owned()).into()),
         ),
         (
-            r#"SELECT CAST("1" AS INTEGER) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast I64;
-                1
-            )),
+            r#"SELECT CAST(1.1 AS INTEGER) AS cast FROM Item"#,
+            Ok(select!(cast I64; 1)),
         ),
         (
-            r#"SELECT CAST("1.1" AS FLOAT) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast F64;
-                1.1
-            )),
+            r#"SELECT CAST(TRUE AS INTEGER) AS cast FROM Item"#,
+            Ok(select!(cast I64; 1)),
         ),
         (
-            r#"SELECT CAST(1 AS TEXT) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast Str;
-                "1".to_string()
-            )),
+            r#"SELECT CAST(NULL AS INTEGER) AS cast FROM Item"#,
+            Ok(select!(cast OptI64; None)),
         ),
         (
-            r#"SELECT CAST(1.1 AS INTEGER) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast I64;
-                1
-            )),
+            r#"SELECT CAST("1.1" AS FLOAT) AS cast FROM Item"#,
+            Ok(select!(cast F64; 1.1)),
         ),
         (
-            r#"SELECT CAST(1.1 AS TEXT) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast Str;
-                "1.1".to_string()
-            )),
+            r#"SELECT CAST(1 AS FLOAT) AS cast FROM Item"#,
+            Ok(select!(cast F64; 1.0)),
         ),
         (
-            r#"SELECT CAST(TRUE AS INTEGER) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast I64;
-                1
-            )),
+            r#"SELECT CAST("foo" AS FLOAT) AS cast FROM Item"#,
+            Err(ValueError::LiteralCastToFloatFailed("foo".to_owned()).into()),
         ),
         (
-            r#"SELECT CAST(TRUE AS TEXT) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast Str;
-                "TRUE".to_string()
-            )),
+            r#"SELECT CAST(TRUE AS FLOAT) AS cast FROM Item"#,
+            Ok(select!(cast F64; 1.0)),
         ),
         (
-            r#"SELECT CAST(1 AS BOOLEAN) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast Bool;
-                true
-            )),
+            r#"SELECT CAST(NULL AS FLOAT) AS cast FROM Item"#,
+            Ok(select!(cast OptF64; None)),
         ),
         (
-            r#"SELECT CAST("TRUE" AS BOOLEAN) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast Bool;
-                true
-            )),
+            r#"SELECT CAST(1 AS TEXT) AS cast FROM Item"#,
+            Ok(select!(cast Str; "1".to_string())),
         ),
         (
-            r#"SELECT CAST(LOWER(number) AS INTEGER) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast I64;
-                1
-            )),
+            r#"SELECT CAST(1.1 AS TEXT) AS cast FROM Item"#,
+            Ok(select!(cast Str; "1.1".to_string())),
         ),
         (
-            r#"SELECT CAST(1 AS FLOAT) AS cast FROM Item LIMIT 1"#,
-            Ok(select!(
-                cast I64; // Ideally should be F64
-                1
-            )),
+            r#"SELECT CAST(TRUE AS TEXT) AS cast FROM Item"#,
+            Ok(select!(cast Str; "TRUE".to_string())),
         ),
         (
-            r#"SELECT CAST("ABC" AS INTEGER) FROM Item LIMIT 1"#,
-            Err(ValueError::FailedToParseNumber.into()),
+            r#"SELECT CAST(NULL AS TEXT) AS cast FROM Item"#,
+            Ok(select!(cast OptStr; None)),
         ),
         (
-            r#"SELECT CAST("$1" AS INTEGER) FROM Item LIMIT 1"#,
-            Err(ValueError::FailedToParseNumber.into()),
+            r#"SELECT CAST(NULL AS NULL) FROM Item"#,
+            Err(ValueError::UnimplementedLiteralCast {
+                data_type: "NULL".to_owned(),
+                literal: "NULL".to_owned(),
+            }
+            .into()),
         ),
+    ];
+
+    for (sql, expected) in test_cases.into_iter() {
+        test!(expected, sql);
+    }
+});
+
+test_case!(cast_value, async move {
+    use Value::*;
+
+    let test_cases = vec![
+        ("CREATE TABLE Item (number TEXT)", Ok(Payload::Create)),
+        (r#"INSERT INTO Item VALUES ("1")"#, Ok(Payload::Insert(1))),
         (
-            r#"SELECT CAST("BLEH" AS BOOLEAN) FROM Item LIMIT 1"#,
-            Err(EvaluateError::ImpossibleCast.into()),
+            r#"SELECT CAST(LOWER(number) AS INTEGER) AS cast FROM Item"#,
+            Ok(select!(cast I64; 1)),
         ),
         (
-            r#"SELECT CAST(number AS BOOLEAN) FROM Item LIMIT 1"#,
+            r#"SELECT CAST(number AS BOOLEAN) FROM Item"#,
             Err(ValueError::ImpossibleCast.into()),
         ),
     ];
+
     for (sql, expected) in test_cases.into_iter() {
         test!(expected, sql);
     }
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index 1cc2ef37..383c6c26 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -67,7 +67,8 @@ macro_rules! generate_tests {
         glue!(error, error::error);
         glue!(filter, filter::filter);
         glue!(function, function::function);
-        glue!(function_cast, function::cast::cast);
+        glue!(function_cast_literal, function::cast::cast_literal);
+        glue!(function_cast_value, function::cast::cast_value);
         glue!(join, join::join);
         glue!(join_blend, join::blend);
         glue!(migrate, migrate::migrate);