From c6746252dadfc00920fe038f2e8c24b21342b39f Mon Sep 17 00:00:00 2001
From: Jung Seonghun <80201773+seonghun-dev@users.noreply.github.com>
Date: Sat, 22 Oct 2022 20:01:46 +0900
Subject: [PATCH] Update CONCAT function behavior - concat with null to return
 NULL (#899)

---
 core/src/executor/evaluate/function.rs | 31 +++++++++++++++++++-------
 test-suite/src/function/concat.rs      | 24 +++++++++++++++-----
 2 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/core/src/executor/evaluate/function.rs b/core/src/executor/evaluate/function.rs
index 7d24c76e6..dcb592dc3 100644
--- a/core/src/executor/evaluate/function.rs
+++ b/core/src/executor/evaluate/function.rs
@@ -5,7 +5,10 @@ use {
         data::Value,
         result::Result,
     },
-    std::cmp::{max, min},
+    std::{
+        cmp::{max, min},
+        ops::ControlFlow,
+    },
     uuid::Uuid,
 };
 
@@ -55,13 +58,25 @@ macro_rules! eval_to_float {
 // --- text ---
 
 pub fn concat(exprs: Vec<Evaluated<'_>>) -> Result<Value> {
-    exprs
-        .into_iter()
-        .map(|expr| expr.try_into())
-        .filter(|value| !matches!(value, Ok(Value::Null)))
-        .try_fold(Value::Str("".to_owned()), |left, right| {
-            Ok(left.concat(&right?))
-        })
+    enum BreakCase {
+        Null,
+        Err(crate::result::Error),
+    }
+
+    let control_flow = exprs.into_iter().map(|expr| expr.try_into()).try_fold(
+        Value::Str(String::new()),
+        |left, right: Result<Value>| match right {
+            Ok(value) if value.is_null() => ControlFlow::Break(BreakCase::Null),
+            Err(err) => ControlFlow::Break(BreakCase::Err(err)),
+            Ok(value) => ControlFlow::Continue(left.concat(&value)),
+        },
+    );
+
+    match control_flow {
+        ControlFlow::Continue(value) => Ok(value),
+        ControlFlow::Break(BreakCase::Null) => Ok(Value::Null),
+        ControlFlow::Break(BreakCase::Err(err)) => Err(err),
+    }
 }
 
 pub fn lower(name: String, expr: Evaluated<'_>) -> Result<Value> {
diff --git a/test-suite/src/function/concat.rs b/test-suite/src/function/concat.rs
index 9634ded04..98d258bfa 100644
--- a/test-suite/src/function/concat.rs
+++ b/test-suite/src/function/concat.rs
@@ -1,6 +1,6 @@
 use {
     crate::*,
-    gluesql_core::{prelude::Value::*, translate::TranslateError},
+    gluesql_core::{data::ValueError, prelude::Value::*, translate::TranslateError},
 };
 
 test_case!(concat, async move {
@@ -37,12 +37,24 @@ test_case!(concat, async move {
 
     test!(
         r#"select concat("ab", "cd", NULL, "ef") as myconcat from Concat;"#,
-        Ok(select!(
-           myconcat
-           Str;
-           "abcdef".to_owned()
-        ))
+        Ok(select_with_null!(myconcat; Null))
     );
+
+    test!(
+        r#"select concat() as myconcat from Concat;"#,
+        Err(TranslateError::FunctionArgsLengthNotMatchingMin {
+            name: "CONCAT".to_owned(),
+            expected_minimum: 1,
+            found: 0
+        }
+        .into())
+    );
+
+    test!(
+        r#"select concat(DATE "2020-06-11", DATE "2020-16-3") as myconcat from Concat;"#,
+        Err(ValueError::FailedToParseDate("2020-16-3".to_owned()).into())
+    );
+
     // test with non string arguments
     test!(
         r#"select concat(123, 456, 3.14) as myconcat from Concat;"#,