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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.