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

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

>> 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.

Fixed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 27ab8f5d7..354f02948 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1130,9 +1130,9 @@ 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;
+               struct Expr *f = sql_expr_new_anon(db, TK_FALSE);
+               if (f != NULL)
+                       f->type = FIELD_TYPE_BOOLEAN;
                return f;
        } else {
                struct Expr *new_expr = sql_expr_new_anon(db, TK_AND);


> 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?

It is leaf (terminal node of tree), so it should be assigned with type.

>> 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).

Yep, you are absolutely right: now we don’t care about zToken
and can operate only on token type. I’ve removed sql_boolean_tokens.

> 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.

I guess this may turn out to be fair. I’ve opened issue:

https://github.com/tarantool/tarantool/issues/4172

> 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:

I guess because execsql implicitly converts true -> 1, false -> 0.
This is quite convenient way since we don’t have to fix test results.
I can open QA issue to avoid this conversion and make tests clearer.

> 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’.

diff --git a/test/sql-tap/tkt3541.test.lua b/test/sql-tap/tkt3541.test.lua
index 05247728d..0d76e7744 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 max(x) = 0 WHEN min(x) > 0 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;
         ]]
     end, {


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

Partially applied (also see diff and explanation above):

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index b520b23d5..a94414ede 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1238,7 +1238,7 @@ 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_anon(pParse->db, tk);
     if (A.pExpr == NULL) {
       pParse->is_aborted = true;
       return;


> ==============================================================================
> 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:
>     **
> 
> ==============================================================================