[tarantool-patches] Re: [PATCH 7/9] sql: make predicates accept and return boolean

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

Thanks for the patch! See 5 comments below.

On 14/04/2019 18:04, Nikita Pettik wrote:
> This patch make following predicates accept and return only values
> of type boolean: IN, EXISTS, OR, AND, NOT, BETWEEN, IS (NULL).
> In terms of approach, it is enough to patch opcodes implementing these
> predicates.
> 
> Part of #3723
> ---
>  src/box/sql/expr.c              | 26 ++++++++++++--------------
>  src/box/sql/parse.y             |  4 +++-
>  src/box/sql/parse_def.c         |  5 +++++
>  src/box/sql/select.c            |  2 +-
>  src/box/sql/sqlInt.h            |  1 +
>  src/box/sql/vdbe.c              | 37 ++++++++++++++-----------------------
>  test/sql-tap/cse.test.lua       |  6 +++---
>  test/sql-tap/e_select1.test.lua |  2 +-
>  test/sql-tap/in1.test.lua       |  6 +++---
>  test/sql-tap/misc3.test.lua     |  2 +-
>  test/sql-tap/select4.test.lua   |  2 +-
>  test/sql-tap/tkt3541.test.lua   |  2 +-
>  test/sql/types.result           | 22 +++++++++++++---------
>  13 files changed, 59 insertions(+), 58 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index e4de472a8..c5ec55de2 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1134,7 +1130,10 @@ sql_and_expr_new(struct sql *db, struct Expr *left_expr,
>  	} else if (exprAlwaysFalse(left_expr) || exprAlwaysFalse(right_expr)) {
>  		sql_expr_delete(db, left_expr, false);
>  		sql_expr_delete(db, right_expr, false);
> -		return sql_expr_new(db, TK_INTEGER, &sqlIntTokens[0]);
> +		struct Expr *f = sql_expr_new(db, TK_FALSE,
> +					      &sql_boolean_tokens[0]);
> +		f->type = FIELD_TYPE_BOOLEAN;

1. No check for f == NULL.

2. Why do you need f->type at all, if anyway you use sql_expr_type()
to get the type? Why isn't this field dropped still?

> +		return f;
>  	} else {
>  		struct Expr *new_expr = sql_expr_new_anon(db, TK_AND);
>  		sqlExprAttachSubtrees(db, new_expr, left_expr, right_expr);
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index f64a84948..b520b23d5 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1237,11 +1237,13 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
>      ** regardless of the value of expr1.
>      */
>      sql_expr_delete(pParse->db, A.pExpr, false);
> -    A.pExpr = sql_expr_new_dequoted(pParse->db, TK_INTEGER, &sqlIntTokens[N]);
> +    int tk = N == 0 ? TK_FALSE : TK_TRUE;
> +    A.pExpr = sql_expr_new_dequoted(pParse->db, tk, &sql_boolean_tokens[N]);

3. Why do you use 'dequoted'? Do you store not normalized names in
sql_boolean_tokens? If you do, then why not to store already
normalized names and use sql_expr_new() here so as to avoid ICU calls?

A normalized name is always uppercased, but sql_boolean_tokens stores
lowercase strings. I tried to do this:

    -    A.pExpr = sql_expr_new_dequoted(pParse->db, tk, &sql_boolean_tokens[N]);
    +    A.pExpr = sql_expr_new(pParse->db, tk, &sql_boolean_tokens[N]);

And the tests passed. Why?

Another question - why boolean constant needs zToken at all? As I understand,
the only reason is that sqlExprCodeTarget parses zToken again even having ready
at hand TK_FALSE and TK_TRUE values. If I am right, you can drop boolean_tokens
array once you've fixed my comment 5 from the main patch (4/9).

The same question could be asked for integer constants stored in struct Expr
and having zToken. The only reason for that is the delayed parsing of the tokens,
as I understand. First they all are stored in Expr.zToken, and then parsed in
CodeTarget. But it would not be necessary if we parsed them right after reading and
storing actual value into Expr.u.iValue. In such a case zToken would be used for
identifiers and strings only, not for constants which I think appear quite often.
The reasoning behind my proposal is that parsing of an integer is likely to be
faster itself than calling ICU normalization functions.

The same can be said about float/double values, but I do not see such a type in
Expr.u.

Of course, all these integer/float-related thoughts are not about this patchset.
Probably, it is worth opening a separate issue.

>      if (A.pExpr == NULL) {
>        pParse->is_aborted = true;
>        return;
>      }
> +    A.pExpr->type = FIELD_TYPE_BOOLEAN;
>    }else if( Y->nExpr==1 ){
>      /* Expressions of the form:
>      **
> diff --git a/test/sql-tap/cse.test.lua b/test/sql-tap/cse.test.lua
> index 4b25f936d..39c1cc4ca 100755
> --- a/test/sql-tap/cse.test.lua
> +++ b/test/sql-tap/cse.test.lua
> @@ -132,7 +132,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "cse-1.7",
>      [[
> -        SELECT a, -a, ~a, NOT a, NOT NOT a, a-a, a+a, a*a, a/a, a FROM t1
> +        SELECT a, -a, ~a, NOT CAST(a AS BOOLEAN), NOT NOT CAST(a AS BOOLEAN), a-a, a+a, a*a, a/a, a FROM t1
>      ]], {
>          -- <cse-1.7>
>          1, -1, -2, 0, 1, 0, 2, 1, 1, 1, 2, -2, -3, 0, 1, 0, 4, 4, 1, 2

4. Above the fourth selected column is 'NOT CAST(a AS BOOLEAN)'. It is either true or
false, but in the result set it is 0. Why? First I thought, that probably 0 == false
in Lua, but it is not as a test showed:

tarantool> 0 == false
---
- false
...

tarantool> 1 == true
---
- false
...

> diff --git a/test/sql-tap/tkt3541.test.lua b/test/sql-tap/tkt3541.test.lua
> index 08a9be557..05247728d 100755
> --- a/test/sql-tap/tkt3541.test.lua
> +++ b/test/sql-tap/tkt3541.test.lua
> @@ -39,7 +39,7 @@ test:do_test(
>      "tkt3541-1.2",
>      function()
>          return test:execsql [[
> -            SELECT CASE NOT max(x) WHEN min(x) THEN 1 ELSE max(x) END FROM t1;
> +            SELECT CASE max(x) = 0 WHEN min(x) > 0 THEN 1 ELSE max(x) END FROM t1;

5. If negative numbers are not treated as FALSE, then I
guess you meant 'min(x) <> 0'.

>          ]]
>      end, {
>          -- <tkt3541-1.2>

My total diff which I am not sure why does not break tests:

==============================================================================
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index c5ec55de2..42b60babd 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1130,10 +1130,7 @@ sql_and_expr_new(struct sql *db, struct Expr *left_expr,
 	} else if (exprAlwaysFalse(left_expr) || exprAlwaysFalse(right_expr)) {
 		sql_expr_delete(db, left_expr, false);
 		sql_expr_delete(db, right_expr, false);
-		struct Expr *f = sql_expr_new(db, TK_FALSE,
-					      &sql_boolean_tokens[0]);
-		f->type = FIELD_TYPE_BOOLEAN;
-		return f;
+		return sql_expr_new_anon(db, TK_FALSE);
 	} else {
 		struct Expr *new_expr = sql_expr_new_anon(db, TK_AND);
 		sqlExprAttachSubtrees(db, new_expr, left_expr, right_expr);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index b520b23d5..f75999c35 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1238,12 +1238,11 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
     */
     sql_expr_delete(pParse->db, A.pExpr, false);
     int tk = N == 0 ? TK_FALSE : TK_TRUE;
-    A.pExpr = sql_expr_new_dequoted(pParse->db, tk, &sql_boolean_tokens[N]);
+    A.pExpr = sql_expr_new(pParse->db, tk, &sql_boolean_tokens[N]);
     if (A.pExpr == NULL) {
       pParse->is_aborted = true;
       return;
     }
-    A.pExpr->type = FIELD_TYPE_BOOLEAN;
   }else if( Y->nExpr==1 ){
     /* Expressions of the form:
     **

==============================================================================