Repository: gluesql/gluesql
Pull request #90 (2020-10-10 05:46:31)

Remove Unreachable error in Row, replace to controllable error.
Remove `RowError::Unreachable` and replace to `ExecuteError::UnsupportedInsertValueType(String)`.
`Unreachable` use in this case was not correct because it was reachable.
```sql
INSERT INTO TableA SELECT id FROM TableA
```
This sql query above can trigger `Unreachable` error.
Now it is replaced by `ExecuteError::UnsupportedInsertValueType(_)`


Patch:
diff --git a/src/data/row.rs b/src/data/row.rs
index 1708eca6..412cfa4c 100644
--- a/src/data/row.rs
+++ b/src/data/row.rs
@@ -2,9 +2,7 @@ use serde::{Deserialize, Serialize};
 use std::fmt::Debug;
 use thiserror::Error;
 
-use sqlparser::ast::{
-    ColumnDef, ColumnOption, ColumnOptionDef, Expr, Ident, Query, SetExpr, Values,
-};
+use sqlparser::ast::{ColumnDef, ColumnOption, ColumnOptionDef, Expr, Ident};
 
 use crate::data::Value;
 use crate::result::Result;
@@ -23,9 +21,6 @@ pub enum RowError {
     #[error("unsupported ast value type")]
     UnsupportedAstValueType,
 
-    #[error("Unreachable")]
-    Unreachable,
-
     #[error("conflict! row cannot be empty")]
     ConflictOnEmptyRow,
 }
@@ -45,67 +40,45 @@ impl Row {
             .ok_or_else(|| RowError::ConflictOnEmptyRow.into())
     }
 
-    pub fn new(
-        column_defs: Vec<ColumnDef>,
-        columns: &[Ident],
-        source: &Query,
-    ) -> Result<Vec<Self>> {
-        let values = match &source.body {
-            SetExpr::Values(Values(values)) => values,
-            _ => {
-                return Err(RowError::Unreachable.into());
-            }
-        };
-
-        if values.is_empty() {
-            return Err(RowError::Unreachable.into());
+    pub fn new(column_defs: &[ColumnDef], columns: &[Ident], values: &[Expr]) -> Result<Self> {
+        if values.len() > column_defs.len() {
+            return Err(RowError::TooManyValues.into());
         }
 
-        values
+        column_defs
             .iter()
-            .map(|values| {
-                if values.len() > column_defs.len() {
-                    return Err(RowError::TooManyValues.into());
-                }
-
-                (&column_defs)
+            .enumerate()
+            .map(|(i, column_def)| {
+                let ColumnDef {
+                    name,
+                    data_type,
+                    options,
+                    ..
+                } = column_def;
+                let name = name.to_string();
+
+                let i = match columns.len() {
+                    0 => Ok(i),
+                    _ => columns
+                        .iter()
+                        .position(|target| target.value == name)
+                        .ok_or_else(|| RowError::LackOfRequiredColumn(name.clone())),
+                }?;
+
+                let literal = values
+                    .get(i)
+                    .ok_or_else(|| RowError::LackOfRequiredValue(name.clone()))?;
+                let nullable = options
                     .iter()
-                    .enumerate()
-                    .map(|(i, column_def)| {
-                        let ColumnDef {
-                            name,
-                            data_type,
-                            options,
-                            ..
-                        } = column_def;
-                        let name = name.to_string();
+                    .any(|ColumnOptionDef { option, .. }| option == &ColumnOption::Null);
 
-                        let i = match columns.len() {
-                            0 => Ok(i),
-                            _ => columns
-                                .iter()
-                                .position(|target| target.value == name)
-                                .ok_or_else(|| RowError::LackOfRequiredColumn(name.clone())),
-                        }?;
-
-                        let literal = values
-                            .get(i)
-                            .ok_or_else(|| RowError::LackOfRequiredValue(name.clone()))?;
-                        let nullable = options
-                            .iter()
-                            .any(|ColumnOptionDef { option, .. }| option == &ColumnOption::Null);
-
-                        match literal {
-                            Expr::Value(literal) => {
-                                Value::from_data_type(&data_type, nullable, literal)
-                            }
-                            Expr::Identifier(Ident { value, .. }) => Ok(Value::Str(value.clone())),
-                            _ => Err(RowError::UnsupportedAstValueType.into()),
-                        }
-                    })
-                    .collect::<Result<_>>()
-                    .map(Self)
+                match literal {
+                    Expr::Value(literal) => Value::from_data_type(&data_type, nullable, literal),
+                    Expr::Identifier(Ident { value, .. }) => Ok(Value::Str(value.clone())),
+                    _ => Err(RowError::UnsupportedAstValueType.into()),
+                }
             })
-            .collect()
+            .collect::<Result<_>>()
+            .map(Self)
     }
 }
diff --git a/src/executor/execute.rs b/src/executor/execute.rs
index eb02c7ef..286d2fc3 100644
--- a/src/executor/execute.rs
+++ b/src/executor/execute.rs
@@ -2,7 +2,7 @@ use serde::Serialize;
 use std::fmt::Debug;
 use thiserror::Error;
 
-use sqlparser::ast::{ObjectType, Statement};
+use sqlparser::ast::{ObjectType, SetExpr, Statement, Values};
 
 use super::fetch::{fetch, fetch_columns};
 use super::filter::Filter;
@@ -20,6 +20,9 @@ pub enum ExecuteError {
 
     #[error("drop type not supported")]
     DropTypeNotSupported,
+
+    #[error("unsupported insert value type: {0}")]
+    UnsupportedInsertValueType(String),
 }
 
 #[derive(Serialize, Debug, PartialEq)]
@@ -130,7 +133,19 @@ fn prepare<'a, T: 'static + Debug>(
         } => {
             let table_name = get_name(table_name)?;
             let Schema { column_defs, .. } = storage.fetch_schema(table_name)?;
-            let rows = Row::new(column_defs, columns, source)?;
+            let values_list = match &source.body {
+                SetExpr::Values(Values(values_list)) => values_list,
+                set_expr => {
+                    return Err(
+                        ExecuteError::UnsupportedInsertValueType(set_expr.to_string()).into(),
+                    );
+                }
+            };
+
+            let rows = values_list
+                .iter()
+                .map(|values| Row::new(&column_defs, columns, values))
+                .collect::<Result<_>>()?;
 
             Ok(Prepared::Insert(table_name, rows))
         }


Test patch:
diff --git a/src/tests/error.rs b/src/tests/error.rs
index 6e4db968..d409f40f 100644
--- a/src/tests/error.rs
+++ b/src/tests/error.rs
@@ -43,6 +43,10 @@ pub fn error(mut tester: impl tests::Tester) {
             RowError::TooManyValues.into(),
             "INSERT INTO TableA VALUES (100), (100, 200);",
         ),
+        (
+            ExecuteError::UnsupportedInsertValueType("SELECT id FROM TableA".to_owned()).into(),
+            "INSERT INTO TableA SELECT id FROM TableA",
+        ),
     ];
 
     test_cases