[tarantool-patches] Re: [PATCH 2/5] sql: fix resulting type calculation for CASE-WHEN stmt
n.pettik
korablev at tarantool.org
Mon Jul 29 02:56:10 MSK 2019
> On 26 Jul 2019, at 01:03, Vladislav Shpilevoy <v.shpilevoy at 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;")
More information about the Tarantool-patches
mailing list