[tarantool-patches] Re: [PATCH 7/9] sql: make predicates accept and return boolean
n.pettik
korablev at tarantool.org
Thu Apr 18 20:55:21 MSK 2019
>> 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:
> **
>
> ==============================================================================
More information about the Tarantool-patches
mailing list