[tarantool-patches] Re: [PATCH 2/5] sql: fix resulting type calculation for CASE-WHEN stmt

["n.pettik" <korablev@tarantool.org>]

> On 26 Jul 2019, at 01:03, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
> On 24/07/2019 13:42, Nikita Pettik wrote:
>> Before this patch, resulting type for CASE-WHEN statement was assumed to
>> be the same as type of argument of first THEN clause. Obviously, it is
>> wrong and could lead to sad consequence (e.g. creating ephemeral table
>> with inconsistent format). To deal with this, we check all THEN
>> arguments: if all of them have the same type, then such type will be
>> resulting of the whole statement; if at least two types are different,
>> we can't determine actual resulting type during compilation stage and
>> assign SCALAR as a most general type in SQL now.
>> 
>> Need for #4206
>> ---
>> src/box/sql/expr.c      | 27 +++++++++++++++++++++------
>> test/sql/types.result   | 35 +++++++++++++++++++++++++++++++++++
>> test/sql/types.test.lua |  8 ++++++++
>> 3 files changed, 64 insertions(+), 6 deletions(-)
>> 
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index d7104d8a0..97f5bd180 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -85,18 +85,33 @@ sql_expr_type(struct Expr *pExpr)
>> 		return sql_type_result(rhs_type, lhs_type);
>> 	case TK_CONCAT:
>> 		return FIELD_TYPE_STRING;
>> -	case TK_CASE:
>> -		assert(pExpr->x.pList->nExpr >= 2);
>> +	case TK_CASE: {
>> +		struct ExprList *cs = pExpr->x.pList;
>> +		assert(cs->nExpr >= 2);
>> 		/*
>> 		 * CASE expression comes at least with one
>> 		 * WHEN and one THEN clauses. So, first
>> 		 * expression always represents WHEN
>> 		 * argument, and the second one - THEN.
>> -		 *
>> -		 * TODO: We must ensure that all THEN clauses
>> -		 * have arguments of the same type.
>> +		 * In case at least one type of THEN argument
>> +		 * is different from others then we can't
>> +		 * determine type of returning value at compiling
>> +		 * stage and set SCALAR (i.e. most general) type.
>> 		 */
>> -		return sql_expr_type(pExpr->x.pList->a[1].pExpr);
>> +		enum field_type ref_type = sql_expr_type(cs->a[1].pExpr);
>> +		for (int i = 1; i <= cs->nExpr / 2; i = i * 2) {
>> +			if (ref_type != sql_expr_type(cs->a[i + 1].pExpr))
>> +				return FIELD_TYPE_SCALAR;
>> +		}
> 
> Something is wrong with that cycle.
> 
> box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 END")
> ---
> - metadata:
>  - name: CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4
>      WHEN 'e' THEN 5 WHEN 'f' THEN 6 END
>    type: scalar
>  rows:
>  - [1]
> ...
> 
> But it is not scalar, it is integer - all 'then's
> here return an integer. Perhaps this is because in
> the cycle you do i*=2 instead of i+=2, so you compare
> not 1 vs 3, 1 vs 5, 1 vs 7, etc, (all 'then' are by
> odd indexes), but 1 vs 2, 1 vs 3, 1 vs 5, 1 vs 9, etc.

Yep, this is obvious bug. Fixed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 97f5bd180..7b3a41c46 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -99,8 +99,8 @@ sql_expr_type(struct Expr *pExpr)
                 * stage and set SCALAR (i.e. most general) type.
                 */
                enum field_type ref_type = sql_expr_type(cs->a[1].pExpr);
-               for (int i = 1; i <= cs->nExpr / 2; i = i * 2) {
-                       if (ref_type != sql_expr_type(cs->a[i + 1].pExpr))
+               for (int i = 3; i < cs->nExpr; i += 2) {
+                       if (ref_type != sql_expr_type(cs->a[i].pExpr))
                                return FIELD_TYPE_SCALAR;
                }
                /*
diff --git a/test/sql/types.result b/test/sql/types.result
index 1db4b980d..d05fa9dfa 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1001,3 +1001,39 @@ box.execute("SELECT CASE 1 WHEN 1 THEN 666 WHEN 2 THEN 123 ELSE 'asd' END")
   rows:
   - [666]
 ...
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 END;")
+---
+- metadata:
+  - name: CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4
+      WHEN 'e' THEN 5 WHEN 'f' THEN 6 END
+    type: integer
+  rows:
+  - [1]
+...
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 'asd' END;")
+---
+- metadata:
+  - name: CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4
+      WHEN 'e' THEN 5 WHEN 'f' THEN 'asd' END
+    type: scalar
+  rows:
+  - [1]
+...
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 ELSE 'asd' END;")
+---
+- metadata:
+  - name: CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4
+      WHEN 'e' THEN 5 WHEN 'f' THEN 6 ELSE 'asd' END
+    type: scalar
+  rows:
+  - [1]
+...
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 ELSE 7 END;")
+---
+- metadata:
+  - name: CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4
+      WHEN 'e' THEN 5 WHEN 'f' THEN 6 ELSE 7 END
+    type: integer
+  rows:
+  - [1]
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index b66a3e068..8fd20f892 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -242,3 +242,7 @@ box.execute("SELECT CASE 1 WHEN 1 THEN x'0000000000' WHEN 2 THEN 'str' END")
 box.execute("SELECT CASE 1 WHEN 1 THEN 666 WHEN 2 THEN 123 END")
 box.execute("SELECT CASE 1 WHEN 1 THEN 666 WHEN 2 THEN 123 ELSE 321 END")
 box.execute("SELECT CASE 1 WHEN 1 THEN 666 WHEN 2 THEN 123 ELSE 'asd' END")
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 END;")
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 'asd' END;")
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 ELSE 'asd' END;")
+box.execute("SELECT CASE 'a' WHEN 'a' THEN 1 WHEN 'b' THEN 2 WHEN 'c' THEN 3 WHEN 'd' THEN 4 WHEN 'e' THEN 5 WHEN 'f' THEN 6 ELSE 7 END;")