From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 708782B694 for ; Thu, 18 Apr 2019 13:55:24 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pFlJS60XvNCC for ; Thu, 18 Apr 2019 13:55:24 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DC2582B65A for ; Thu, 18 Apr 2019 13:55:23 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH 7/9] sql: make predicates accept and return boolean From: "n.pettik" In-Reply-To: <29403d26-5619-32ec-d2a6-474cc5256994@tarantool.org> Date: Thu, 18 Apr 2019 20:55:21 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <42E59DBE-FC8E-4048-8A82-1DACACFC8F74@tarantool.org> References: <8a92ae4cb6030c442fc540fa0ea07d9e6daa4e98.1555252410.git.korablev@tarantool.org> <29403d26-5619-32ec-d2a6-474cc5256994@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >> 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 =3D sql_expr_new(db, TK_FALSE, >> + &sql_boolean_tokens[0]); >> + f->type =3D FIELD_TYPE_BOOLEAN; >=20 > 1. No check for f =3D=3D 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 =3D sql_expr_new(db, TK_FALSE, - &sql_boolean_tokens[0]); - f->type =3D FIELD_TYPE_BOOLEAN; + struct Expr *f =3D sql_expr_new_anon(db, TK_FALSE); + if (f !=3D NULL) + f->type =3D FIELD_TYPE_BOOLEAN; return f; } else { struct Expr *new_expr =3D 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) ::=3D 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 =3D sql_expr_new_dequoted(pParse->db, TK_INTEGER, = &sqlIntTokens[N]); >> + int tk =3D N =3D=3D 0 ? TK_FALSE : TK_TRUE; >> + A.pExpr =3D sql_expr_new_dequoted(pParse->db, tk, = &sql_boolean_tokens[N]); >=20 > 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? >=20 > A normalized name is always uppercased, but sql_boolean_tokens stores > lowercase strings. I tried to do this: >=20 > - A.pExpr =3D sql_expr_new_dequoted(pParse->db, tk, = &sql_boolean_tokens[N]); > + A.pExpr =3D sql_expr_new(pParse->db, tk, = &sql_boolean_tokens[N]); >=20 > 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=E2=80=99t care about zToken and can operate only on token type. I=E2=80=99ve 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. >=20 > 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=E2=80=99ve 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. >=20 >> if (A.pExpr =3D=3D NULL) { >> pParse->is_aborted =3D true; >> return; >> } >> + A.pExpr->type =3D FIELD_TYPE_BOOLEAN; >> }else if( Y->nExpr=3D=3D1 ){ >> /* 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 >> ]], { >> -- >> 1, -1, -2, 0, 1, 0, 2, 1, 1, 1, 2, -2, -3, 0, 1, 0, 4, 4, 1, = 2 >=20 > 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 =3D=3D 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=E2=80=99t have to fix test = results. I can open QA issue to avoid this conversion and make tests clearer. > tarantool> 0 =3D=3D false > --- > - false > ... >=20 > tarantool> 1 =3D=3D true > --- > - false > ... >=20 >> 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) =3D 0 WHEN min(x) > 0 THEN 1 ELSE = max(x) END FROM t1; >=20 > 5. If negative numbers are not treated as FALSE, then I > guess you meant 'min(x) <> 0=E2=80=99. 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) =3D 0 WHEN min(x) > 0 THEN 1 ELSE max(x) = END FROM t1; + SELECT CASE max(x) =3D 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) ::=3D expr(A) in_op(N) LP exprlist(Y) = RP(E). [IN] { */ sql_expr_delete(pParse->db, A.pExpr, false); int tk =3D N =3D=3D 0 ? TK_FALSE : TK_TRUE; - A.pExpr =3D sql_expr_new_dequoted(pParse->db, tk, = &sql_boolean_tokens[N]); + A.pExpr =3D sql_expr_new_anon(pParse->db, tk); if (A.pExpr =3D=3D NULL) { pParse->is_aborted =3D true; return; > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > 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 =3D sql_expr_new(db, TK_FALSE, > - &sql_boolean_tokens[0]); > - f->type =3D FIELD_TYPE_BOOLEAN; > - return f; > + return sql_expr_new_anon(db, TK_FALSE); > } else { > struct Expr *new_expr =3D 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) ::=3D expr(A) in_op(N) LP exprlist(Y) = RP(E). [IN] { > */ > sql_expr_delete(pParse->db, A.pExpr, false); > int tk =3D N =3D=3D 0 ? TK_FALSE : TK_TRUE; > - A.pExpr =3D sql_expr_new_dequoted(pParse->db, tk, = &sql_boolean_tokens[N]); > + A.pExpr =3D sql_expr_new(pParse->db, tk, &sql_boolean_tokens[N]); > if (A.pExpr =3D=3D NULL) { > pParse->is_aborted =3D true; > return; > } > - A.pExpr->type =3D FIELD_TYPE_BOOLEAN; > }else if( Y->nExpr=3D=3D1 ){ > /* Expressions of the form: > ** >=20 > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D