Re: [Tarantool-patches] [PATCH 5/6] sql: remove implicit cast from string for comparison

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

Thanks for the patch!

>>> index 854ed774f..2a18b17be 100755
>>> --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
>>> +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
>>> @@ -83,23 +83,23 @@ test:do_execsql_test(
>>>           -- </1.5>
>>>       })
>>>   -test:do_execsql_test(
>>> +test:do_catchsql_test(
>>>       2.1,
>>>       [[
>>>           SELECT x FROM t1 WHERE x IN (1);
>>>       ]], {
>>>           -- <2.1>
>>> -        "1"
>>> +        1,"Type mismatch: can not convert 1 to numeric"
>> 3. Can tests in this file be fixed to return the same results
>> as before?
> I fixed this in most cases, but am not sure if this is correct.
> I think most tests are out of date.

Yeah, looks like the test file becomes mostly useless. It now
tests rather explicit CAST than implicit cast. The previous
version was better. At least it covered implicit cast errors.

I would try to rollback to the previous version, remove obviously
duplicated or not needed test cases, and see what is left.
Probably the whole test file is useless.

See 6 comments below.

> sql: remove implicit cast from string for comparison
> 
> This patch removes implicit cast from strings to numbers for
> comparison.
> 
> Closes #4230
> 
> @TarantoolBot document
> Title: remove implicit cast between strings and numbers
> 
> This patch-set removes implicit cast from string to number and
> from number to string.
> 
> Example:
> 
> For comparison:
> 
> tarantool> box.execute([[SELECT '1' > 0;]])
> ---
> - null
> - 'Type mismatch: can not convert 1 to numeric'
> ...
> 
> tarantool> box.execute([[SELECT "id" FROM "_space" WHERE '1' > "id";]])
> ---
> - null
> - 'Type mismatch: can not convert text to unsigned'
> ...
> 
> For assignment:
> 
> tarantool> box.execute([[CREATE TABLE t1(i INT PRIMARY KEY);]])
> tarantool> box.execute([[INSERT INTO t1 VALUES ('1');]])
> ---
> - null
> - 'Type mismatch: can not convert text to integer'
> ...
> 
> tarantool> box.execute([[CREATE TABLE t2(t text PRIMARY KEY);]])
> tarantool> box.execute([[INSERT INTO t2 VALUES (1);]])
> ---
> - null
> - 'Type mismatch: can not convert unsigned to string'
> ...

1. I suggest to wrap code samples into ``` to produce more
readable github markdown in the resulting doc issue.

> review fix

2. Hm.

> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 6d8768865..1d7c76670 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -335,72 +335,6 @@ disableTerm(WhereLevel * pLevel, WhereTerm * pTerm)
>      }
>  }
> 
> -/**
> - * Code an OP_ApplyType opcode to apply the column type string
> - * @types to the n registers starting at @base.
> - *
> - * As an optimization, SCALAR entries (which are no-ops) at the
> - * beginning and end of @types are ignored.  If all entries in
> - * @types are SCALAR, then no code gets generated.
> - *
> - * This routine makes its own copy of @types so that the caller is
> - * free to modify @types after this routine returns.
> - */
> -static void
> -emit_apply_type(Parse *pParse, int base, int n, enum field_type *types)
> -{
> -    Vdbe *v = pParse->pVdbe;
> -    if (types == NULL) {
> -        assert(pParse->db->mallocFailed);
> -        return;
> -    }
> -    assert(v != 0);
> -
> -    /*
> -     * Adjust base and n to skip over SCALAR entries at the
> -     * beginning and end of the type sequence.
> -     */
> -    while (n > 0 && types[0] == FIELD_TYPE_SCALAR) {
> -        n--;
> -        base++;
> -        types++;
> -    }
> -    while (n > 1 && types[n - 1] == FIELD_TYPE_SCALAR) {
> -        n--;
> -    }
> -
> -    if (n > 0) {
> -        enum field_type *types_dup = field_type_sequence_dup(pParse,
> -                                     types, n);

3. This function is now unused and can be deleted.

> -        sqlVdbeAddOp4(v, OP_ApplyType, base, n, 0,
> -                  (char *) types_dup, P4_DYNAMIC);
> -        sql_expr_type_cache_change(pParse, base, n);
> -    }
> -}
> -
> -/**
> - * Expression @rhs, which is the RHS of a comparison operation, is
> - * either a vector of n elements or, if n==1, a scalar expression.
> - * Before the comparison operation, types @types are to be applied
> - * to the @rhs values. This function modifies entries within the
> - * field sequence to SCALAR if either:
> - *
> - *   * the comparison will be performed with no type, or
> - *   * the type change in @types is guaranteed not to change the value.
> - */
> -static void
> -expr_cmp_update_rhs_type(struct Expr *rhs, int n, enum field_type *types)
> -{
> -    for (int i = 0; i < n; i++) {
> -        Expr *p = sqlVectorFieldSubexpr(rhs, i);
> -        enum field_type expr_type = sql_expr_type(p);
> -        if (sql_type_result(expr_type, types[i]) == FIELD_TYPE_SCALAR ||
> -            sql_expr_needs_no_type_change(p, types[i])) {

4. Ditto (removed in the next commit, up to you if want to remove
here).

> -            types[i] = FIELD_TYPE_SCALAR;
> -        }
> -    }
> -}
> diff --git a/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua
> new file mode 100755
> index 000000000..ef4127e0e
> --- /dev/null
> +++ b/test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua
> @@ -0,0 +1,78 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(8)
> +
> +--
> +-- Make sure that there is no implicit cast between string and
> +-- number.
> +--
> +test:do_catchsql_test(
> +    "gh-4230-1",
> +    [[
> +        SELECT '1' > 0;
> +    ]], {
> +        1, "Type mismatch: can not convert 1 to numeric"
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-4230-2",
> +    [[
> +        SELECT 0 > '1';
> +    ]], {
> +        1, "Type mismatch: can not convert 1 to numeric"

5. The error messages are really weird. We should probably wrap
string value into quoutes. Or print it as 'text'. Without a
value. Not necessarily here. You can create an issue for that,
and do it later.

> diff --git a/test/sql-tap/tkt-f973c7ac31.test.lua b/test/sql-tap/tkt-f973c7ac31.test.lua
> index 82bdb52f8..604a7e6bb 100755
> --- a/test/sql-tap/tkt-f973c7ac31.test.lua
> +++ b/test/sql-tap/tkt-f973c7ac31.test.lua
> @@ -39,9 +39,8 @@ for tn, sql in ipairs(sqls) do
>      test:do_execsql_test(
>          "tkt-f973c7ac3-1."..tn..".1",
>          [[
> -            SELECT c1,c2 FROM t WHERE c1 = 5 AND c2>0 AND c2<='2' ORDER BY c2 DESC
> +            SELECT c1,c2 FROM t WHERE c1 = 5 AND c2>0 AND CAST(c2 AS STRING)<='2' ORDER BY c2 DESC

6. On the branch I see trailing whitespaces in this file.

>          ]], {
> -
>          })
>