Repository: gluesql/gluesql
Pull request #181 (2021-03-17 13:40:31)

Remove Value::from_data_type, merge to TryFromLiteral trait for Value
Remove `Value::from_data_type` and merge to `TryFromLiteral` trait which has impl for `Value`.

However, this is still a temporal refactoring.
Once after we use `evaluate` to handle `expr`, then we can remove `Value::from_expr`.


Patch:
diff --git a/src/data/literal.rs b/src/data/literal.rs
index 53616c9e..c816f372 100644
--- a/src/data/literal.rs
+++ b/src/data/literal.rs
@@ -1,6 +1,6 @@
 use {
     crate::result::{Error, Result},
-    sqlparser::ast::Value as AstValue,
+    sqlparser::ast::{Expr, Ident, Value as AstValue},
     std::{borrow::Cow, cmp::Ordering, convert::TryFrom, fmt::Debug},
     Literal::*,
 };
@@ -9,9 +9,12 @@ use {serde::Serialize, thiserror::Error};
 
 #[derive(Error, Serialize, Debug, PartialEq)]
 pub enum LiteralError {
-    #[error("Unsupported literal type: {0}")]
+    #[error("unsupported literal type: {0}")]
     UnsupportedLiteralType(String),
 
+    #[error("unsupported expr: {0}")]
+    UnsupportedExpr(String),
+
     #[error("unsupported literal binary arithmetic between {0} and {1}")]
     UnsupportedBinaryArithmetic(String, String),
 
@@ -51,6 +54,18 @@ impl<'a> TryFrom<&'a AstValue> for Literal<'a> {
     }
 }
 
+impl<'a> TryFrom<&'a Expr> for Literal<'a> {
+    type Error = Error;
+
+    fn try_from(expr: &'a Expr) -> Result<Self> {
+        match expr {
+            Expr::Value(literal) => Literal::try_from(literal),
+            Expr::Identifier(Ident { value, .. }) => Ok(Literal::Text(Cow::Borrowed(value))),
+            _ => Err(LiteralError::UnsupportedExpr(expr.to_string()).into()),
+        }
+    }
+}
+
 impl PartialEq<Literal<'_>> for Literal<'_> {
     fn eq(&self, other: &Literal) -> bool {
         match (self, other) {
diff --git a/src/data/value/error.rs b/src/data/value/error.rs
index f3001776..4d47c7e9 100644
--- a/src/data/value/error.rs
+++ b/src/data/value/error.rs
@@ -2,14 +2,8 @@ use {serde::Serialize, std::fmt::Debug, thiserror::Error};
 
 #[derive(Error, Serialize, Debug, PartialEq)]
 pub enum ValueError {
-    #[error("sql type not supported yet")]
-    SqlTypeNotSupported,
-
-    #[error("literal not supported yet")]
-    LiteralNotSupported,
-
-    #[error("ast expr not supported: {0}")]
-    ExprNotSupported(String),
+    #[error("literal: {literal} is incompatible with data type: {data_type}")]
+    IncompatibleLiteralForDataType { data_type: String, literal: String },
 
     #[error("failed to parse number")]
     FailedToParseNumber,
@@ -38,6 +32,9 @@ pub enum ValueError {
     #[error("unary minus operation for non numeric value")]
     UnaryMinusOnNonNumeric,
 
+    #[error("unreachable failure on parsing number")]
+    UnreachableNumberParsing,
+
     #[error("floating columns cannot be set to unique constraint")]
     ConflictOnFloatWithUniqueConstraint,
 
diff --git a/src/data/value/literal.rs b/src/data/value/literal.rs
index fc7aed74..0420e16c 100644
--- a/src/data/value/literal.rs
+++ b/src/data/value/literal.rs
@@ -89,11 +89,38 @@ impl TryFrom<Literal<'_>> for Value {
 
 pub trait TryFromLiteral {
     fn try_from_literal(data_type: &DataType, literal: &Literal<'_>) -> Result<Value>;
+
+    fn try_cast_from_literal(data_type: &DataType, literal: &Literal<'_>) -> Result<Value>;
 }
 
 impl TryFromLiteral for Value {
     fn try_from_literal(data_type: &DataType, literal: &Literal<'_>) -> Result<Value> {
         match (data_type, literal) {
+            (DataType::Boolean, Literal::Boolean(v)) => Ok(Value::Bool(*v)),
+            (DataType::Int, Literal::Number(v)) => v
+                .parse::<i64>()
+                .map(Value::I64)
+                .map_err(|_| ValueError::FailedToParseNumber.into()),
+            (DataType::Float(_), Literal::Number(v)) => v
+                .parse::<f64>()
+                .map(Value::F64)
+                .map_err(|_| ValueError::UnreachableNumberParsing.into()),
+            (DataType::Text, Literal::Text(v)) => Ok(Value::Str(v.to_string())),
+            (DataType::Boolean, Literal::Null)
+            | (DataType::Int, Literal::Null)
+            | (DataType::Float(_), Literal::Null)
+            | (DataType::Text, Literal::Null) => Ok(Value::Null),
+            _ => Err(ValueError::IncompatibleLiteralForDataType {
+                data_type: data_type.to_string(),
+                literal: format!("{:?}", literal),
+            }
+            .into()),
+        }
+    }
+
+    fn try_cast_from_literal(data_type: &DataType, literal: &Literal<'_>) -> Result<Value> {
+        match (data_type, literal) {
+            (DataType::Boolean, Literal::Boolean(v)) => Ok(Value::Bool(*v)),
             (DataType::Boolean, Literal::Text(v)) | (DataType::Boolean, Literal::Number(v)) => {
                 match v.to_uppercase().as_str() {
                     "TRUE" | "1" => Ok(Value::Bool(true)),
diff --git a/src/data/value/mod.rs b/src/data/value/mod.rs
index 59335610..4353fbd9 100644
--- a/src/data/value/mod.rs
+++ b/src/data/value/mod.rs
@@ -1,9 +1,8 @@
 use {
     super::Literal,
     crate::result::Result,
-    boolinator::Boolinator,
     serde::{Deserialize, Serialize},
-    sqlparser::ast::{DataType, Expr, Ident},
+    sqlparser::ast::{DataType, Expr},
     std::{cmp::Ordering, convert::TryFrom, fmt::Debug},
 };
 
@@ -67,13 +66,14 @@ impl BoolToValue for bool {
 
 impl Value {
     pub fn from_expr(data_type: &DataType, nullable: bool, expr: &Expr) -> Result<Self> {
-        match expr {
-            Expr::Value(literal) => {
-                Value::from_data_type(&data_type, nullable, Literal::try_from(literal)?)
-            }
-            Expr::Identifier(Ident { value, .. }) => Ok(Value::Str(value.clone())),
-            _ => Err(ValueError::ExprNotSupported(expr.to_string()).into()),
+        let literal = Literal::try_from(expr)?;
+        let value = Value::try_from_literal(&data_type, &literal)?;
+
+        if !nullable && matches!(value, Value::Null) {
+            return Err(ValueError::NullValueOnNotNullField.into());
         }
+
+        Ok(value)
     }
 
     pub fn is_same_as_data_type(&self, data_type: &DataType) -> bool {
@@ -90,32 +90,6 @@ impl Value {
         )
     }
 
-    pub fn from_data_type(
-        data_type: &DataType,
-        nullable: bool,
-        literal: Literal<'_>,
-    ) -> Result<Self> {
-        match (data_type, literal) {
-            (DataType::Int, Literal::Number(v)) => v
-                .parse()
-                .map(Value::I64)
-                .map_err(|_| ValueError::FailedToParseNumber.into()),
-            (DataType::Float(_), Literal::Number(v)) => v
-                .parse()
-                .map(Value::F64)
-                .map_err(|_| ValueError::FailedToParseNumber.into()),
-            (DataType::Boolean, Literal::Boolean(v)) => Ok(Value::Bool(v)),
-            (DataType::Text, Literal::Text(v)) => Ok(Value::Str(v.into_owned())),
-            (DataType::Int, Literal::Null)
-            | (DataType::Float(_), Literal::Null)
-            | (DataType::Boolean, Literal::Null)
-            | (DataType::Text, Literal::Null) => {
-                nullable.as_result(Value::Null, ValueError::NullValueOnNotNullField.into())
-            }
-            _ => Err(ValueError::SqlTypeNotSupported.into()),
-        }
-    }
-
     pub fn cast(&self, data_type: &DataType) -> Result<Self> {
         match (data_type, self) {
             // Same as
diff --git a/src/executor/evaluate/evaluated.rs b/src/executor/evaluate/evaluated.rs
index 0e1b006e..b8cb140f 100644
--- a/src/executor/evaluate/evaluated.rs
+++ b/src/executor/evaluate/evaluated.rs
@@ -109,7 +109,7 @@ impl<'a> Evaluated<'a> {
     }
 
     pub fn cast(self, data_type: &DataType) -> Result<Evaluated<'a>> {
-        let cast_literal = |literal: &Literal| Value::try_from_literal(data_type, literal);
+        let cast_literal = |literal: &Literal| Value::try_cast_from_literal(data_type, literal);
         let cast_value = |value: &Value| value.cast(data_type);
 
         match self {


Test patch:
diff --git a/src/tests/alter_table.rs b/src/tests/alter_table.rs
index 9a02e39a..6b46b424 100644
--- a/src/tests/alter_table.rs
+++ b/src/tests/alter_table.rs
@@ -80,7 +80,7 @@ test_case!(add_drop, async move {
         ),
         (
             "ALTER TABLE Foo ADD COLUMN something INTEGER DEFAULT (SELECT id FROM Bar LIMIT 1)",
-            Err(ValueError::ExprNotSupported("(SELECT id FROM Bar LIMIT 1)".to_owned()).into()),
+            Err(LiteralError::UnsupportedExpr("(SELECT id FROM Bar LIMIT 1)".to_owned()).into()),
         ),
         (
             "ALTER TABLE Foo DROP COLUMN IF EXISTS something;",
diff --git a/src/tests/migrate.rs b/src/tests/migrate.rs
index 1347dc5a..170a6e0d 100644
--- a/src/tests/migrate.rs
+++ b/src/tests/migrate.rs
@@ -21,12 +21,12 @@ test_case!(migrate, async move {
 
     let error_cases = vec![
         (
-            ValueError::ExprNotSupported("3 * 2".to_owned()).into(),
+            LiteralError::UnsupportedExpr("3 * 2".to_owned()).into(),
             "INSERT INTO Test (id, num) VALUES (3 * 2, 1);",
         ),
         (
             ValueError::FailedToParseNumber.into(),
-            "INSERT INTO Test (id, num) VALUES (1.1, 1);",
+            r#"INSERT INTO Test (id, num, name) VALUES (1.1, 1, "good");"#,
         ),
         (
             EvaluateError::UnsupportedCompoundIdentifier("Here.User.id".to_owned()).into(),
diff --git a/src/tests/nullable.rs b/src/tests/nullable.rs
index 4e2dbd90..89692fba 100644
--- a/src/tests/nullable.rs
+++ b/src/tests/nullable.rs
@@ -230,7 +230,7 @@ CREATE TABLE Test (
             )),
         ),
         (
-            "INSERT INTO Test VALUES (1, NULL)",
+            r#"INSERT INTO Test VALUES (1, NULL, "ok")"#,
             Err(ValueError::NullValueOnNotNullField.into()),
         ),
     ];
diff --git a/src/tests/validate/types.rs b/src/tests/validate/types.rs
index fc4c373a..c91d7d03 100644
--- a/src/tests/validate/types.rs
+++ b/src/tests/validate/types.rs
@@ -1,21 +1,24 @@
-use crate::*;
+use {crate::*, std::borrow::Cow};
 
 test_case!(types, async move {
     run!("CREATE TABLE TableB (id BOOL);");
+    run!("CREATE TABLE TableC (uid INTEGER UNIQUE);");
+    run!("INSERT INTO TableC (uid) VALUES (1);");
     test!(
-        Err(ValueError::SqlTypeNotSupported.into()),
-        "INSERT INTO TableB (id) VALUES (0);"
+        Err(ValidateError::IncompatibleTypeOnTypedField {
+            attempted_value: format!("{:?}", Value::I64(1)),
+            column_name: "id".to_owned(),
+            column_type: "BOOL".to_owned(),
+        }
+        .into()),
+        "INSERT INTO TableB SELECT uid FROM TableC;"
     );
-
-    run!("CREATE TABLE TableC (id INTEGER UNIQUE);");
-    run!("INSERT INTO TableC (id) VALUES (1);");
     test!(
-        Err(ValidateError::IncompatibleTypeOnTypedField {
-            attempted_value: "Str(\"A\")".to_string(),
-            column_name: "id".to_string(),
-            column_type: "INT".to_string()
+        Err(ValueError::IncompatibleLiteralForDataType {
+            data_type: "INT".to_owned(),
+            literal: format!("{:?}", data::Literal::Text(Cow::Owned("A".to_owned()))),
         }
         .into()),
-        "INSERT INTO TableC (id) VALUES (\"A\")"
+        "INSERT INTO TableC (uid) VALUES (\"A\")"
     );
 });