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 A83062B937 for ; Tue, 16 Apr 2019 10:12:59 -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 ZykA8P7uI5Gz for ; Tue, 16 Apr 2019 10:12:59 -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 0AD4F2A28A for ; Tue, 16 Apr 2019 10:12:53 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 7/9] sql: make predicates accept and return boolean References: <8a92ae4cb6030c442fc540fa0ea07d9e6daa4e98.1555252410.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <29403d26-5619-32ec-d2a6-474cc5256994@tarantool.org> Date: Tue, 16 Apr 2019 17:12:50 +0300 MIME-Version: 1.0 In-Reply-To: <8a92ae4cb6030c442fc540fa0ea07d9e6daa4e98.1555252410.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Nikita Pettik Cc: kostja@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 > ]], { > -- > 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, { > -- 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: ** ==============================================================================