[tarantool-patches] Re: [PATCH 9/9] sql: make <search condition> accept only boolean
n.pettik
korablev at tarantool.org
Tue Apr 23 22:59:02 MSK 2019
> On 18/04/2019 20:55, n.pettik wrote:
>>
>>>> <search condition> is a predicate used as a part of WHERE and
>>>> JOIN clauses. ANSI SQL states that <search condition> must
>>>> accept only boolean arguments. In our SQL it is implemented as
>>>> bytecode instruction OP_If which in turn carries out logic of
>>>> conditional jump. Since it can be involved in executing other routines
>>>> different from <search condition>,
>>>
>>> 1. Which other routines? What is a valid case of OP_If with non-boolean
>>> value in check?
>>
>> For instance, to verify that register containing LIMIT value is > 0.
>
> Yes, and this is almost the only case. What is more, it happens only once
> per request, to check if LIMIT == 0 initially. Further it is decremented
> and checked via OP_IfNotZero and OP_DecrJumpZero.
>
>> It is quite hard to track values which come to this opcode, so we
>> can’t be sure that it always accepts booleans.
>
> It is hard, but without it
>
> 1) You can't be sure, that really all the search conditions
> are checked to be booleans;
>
> 2) It makes OP_If/IfNot slower, and they are called repeatedly in
> requests;
One branching worth nothing. Below you suggest to split opcode
into two (fix me if I’m wrong), which in turn affects performance way much more.
> 3) It adds one more flag SQL_BOOLREQ, which looks very crutchy.
IMHO it is matter of taste. Anyway, removed this flag.
> I tried myself to disable non-bool for OP_If/IfNot, and I fixed
> LIMIT cases. Then I've found CASE-WHEN also uses OP_If/IfNot in
> this distorted way.
>
> tarantool> box.execute("SELECT CASE WHEN 0 THEN 'zero' WHEN 1 THEN 'one' END;”)
Fixed.
> It violates the standard. "Information technology —
> Database languages — SQL — Part 2: Foundation (SQL/Foundation)",
> 2011, page 230.
>
> 'WHEN' is a search condition, but I've used '1', not 'true'.
> Also I tested it on PostgreSQL - they raise an error, so it is
> both standard and practically used way.
>
> Below are my fixes for LIMIT and a small obvious refactoring,
> but they are *not on the branch* - not all the tests pass when I
> start banning non-bools in OP_If/IfNot.
I’ve fixed that.
> And I do not like these temporary registers.
They seem OK to me.
> Probably we could somehow reuse OP_IfNotZero,
> for example, via having it refactored to OP_CheckZero which would
> be able to jump both if the target is zero and if not. For example,
> it could look like this:
>
> /**
> * P1 - a register with value to check if it equals zero.
> * P3 - if 1, then check r[P1] == 0, if 0, then check r[P1] != 0.
> * If the check is successful, then jump to P2.
> *
> * if (r[P1] == 0) == P3 then jump p2
> */
>
> But I do not know what to do with its current version decrementing
> r[P1]. My ideas were about splitting 'decrement' operation from
> zero-checking. Then we could do these replacements:
>
> OP_IfNotZero -> OP_CheckZero, OP_Decrement
>
> OP_DecrJumpZero -> OP_Decrement, OP_CheckZero
>
> OP_IfNot -> OP_CheckZero (where it is used inappropriately)
>
> And now we drop OP_IfNotZero and OP_DecrJumpZero, and forbid
> non-bool in OP_IfNot/OP_If. Perhaps we will find more standard
> violations.
>
> ==================================================================
I’ve managed to make OP_If accept only booleans without splitting
opcodes. Diff (including your changes):
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3c4cde2c8..86fc28606 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1094,24 +1094,16 @@ sqlPExprAddSelect(Parse * pParse, Expr * pExpr, Select * pSelect)
* LEFT JOIN, then we cannot determine at compile-time whether or not
* is it true or false, so always return 0.
*/
-static int
+static inline bool
exprAlwaysTrue(Expr * p)
{
- if (ExprHasProperty(p, EP_FromJoin))
- return 0;
- if (p->op == TK_TRUE)
- return 1;
- return 0;
+ return !ExprHasProperty(p, EP_FromJoin) && p->op == TK_TRUE;
}
-static int
+static inline bool
exprAlwaysFalse(Expr * p)
{
- if (ExprHasProperty(p, EP_FromJoin))
- return 0;
- if (p->op == TK_FALSE)
- return 1;
- return 0;
+ return !ExprHasProperty(p, EP_FromJoin) && p->op == TK_FALSE;
}
struct Expr *
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 428e3a92f..280391040 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -769,8 +769,10 @@ pushOntoSorter(Parse * pParse, /* Parser context */
pParse->nMem += pSort->nOBSat;
nKey = nExpr - pSort->nOBSat + bSeq;
if (bSeq) {
- addrFirst =
- sqlVdbeAddOp1(v, OP_IfNot, regBase + nExpr);
+ int r1 = sqlGetTempReg(pParse);
+ sqlVdbeAddOp2(v, OP_Integer, 0, r1);
+ addrFirst = sqlVdbeAddOp3(v, OP_Eq, r1, 0, regBase + nExpr);
+ sqlReleaseTempReg(pParse, r1);
} else {
addrFirst =
sqlVdbeAddOp1(v, OP_SequenceTest,
@@ -799,9 +801,10 @@ pushOntoSorter(Parse * pParse, /* Parser context */
pSort->labelBkOut);
sqlVdbeAddOp1(v, OP_ResetSorter, pSort->iECursor);
if (iLimit) {
- sqlVdbeAddOp2(v, OP_IfNot, iLimit,
- pSort->labelDone);
- VdbeCoverage(v);
+ int r1 = sqlGetTempReg(pParse);
+ sqlVdbeAddOp2(v, OP_Integer, 0, r1);
+ sqlVdbeAddOp3(v, OP_Eq, r1, pSort->labelDone, iLimit);
+ sqlReleaseTempReg(pParse, r1);
}
sqlVdbeJumpHere(v, addrFirst);
sqlExprCodeMove(pParse, regBase, regPrevKey, pSort->nOBSat);
@@ -2115,11 +2118,10 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
P4_STATIC);
sqlVdbeResolveLabel(v, positive_limit_label);
- sqlReleaseTempReg(pParse, r1);
VdbeCoverage(v);
VdbeComment((v, "LIMIT counter"));
- sqlVdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
- VdbeCoverage(v);
+ sqlVdbeAddOp3(v, OP_Eq, r1, iBreak, iLimit);
+ sqlReleaseTempReg(pParse, r1);
if ((p->selFlags & SF_SingleRow) != 0) {
if (ExprHasProperty(p->pLimit, EP_System)) {
@@ -2673,10 +2675,11 @@ multiSelect(Parse * pParse, /* Parsing context */
p->iLimit = pPrior->iLimit;
p->iOffset = pPrior->iOffset;
if (p->iLimit) {
- addr =
- sqlVdbeAddOp1(v, OP_IfNot,
- p->iLimit);
- VdbeCoverage(v);
+ int r1 = sqlGetTempReg(pParse);
+ sqlVdbeAddOp2(v, OP_Integer, 0, r1);
+ addr = sqlVdbeAddOp3(v, OP_Eq, r1, 0,
+ p->iLimit);
+ sqlReleaseTempReg(pParse, r1);
VdbeComment((v,
"Jump ahead if LIMIT reached"));
if (p->iOffset) {
@@ -3047,7 +3050,7 @@ generateOutputSubroutine(struct Parse *parse, struct Select *p,
sqlVdbeJumpHere(v, addr1);
sqlVdbeAddOp3(v, OP_Copy, in->iSdst, reg_prev + 1,
in->nSdst - 1);
- sqlVdbeAddOp2(v, OP_Integer, 1, reg_prev);
+ sqlVdbeAddOp2(v, OP_Bool, true, reg_prev);
}
if (parse->db->mallocFailed)
return 0;
@@ -3370,7 +3373,7 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */
assert(nOrderBy >= expr_count || db->mallocFailed);
regPrev = pParse->nMem + 1;
pParse->nMem += expr_count + 1;
- sqlVdbeAddOp2(v, OP_Integer, 0, regPrev);
+ sqlVdbeAddOp2(v, OP_Bool, 0, regPrev);
key_info_dup = sql_key_info_new(db, expr_count);
if (key_info_dup != NULL) {
for (int i = 0; i < expr_count; i++) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 51d30ee1b..b22598b67 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1721,8 +1721,8 @@ integer_overflow:
* functions.
*
* If P1 is not zero, then it is a register that a subsequent min() or
- * max() aggregate will set to 1 if the current row is not the minimum or
- * maximum. The P1 register is initialized to 0 by this instruction.
+ * max() aggregate will set to true if the current row is not the minimum or
+ * maximum. The P1 register is initialized to false by this instruction.
*
* The interface used by the implementation of the aforementioned functions
* to retrieve the collation sequence set by this opcode is not available
@@ -1731,7 +1731,7 @@ integer_overflow:
case OP_CollSeq: {
assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl == NULL);
if (pOp->p1) {
- sqlVdbeMemSetInt64(&aMem[pOp->p1], 0);
+ mem_set_bool(&aMem[pOp->p1], false);
}
break;
}
@@ -2563,21 +2563,10 @@ case OP_IfNot: { /* jump, in1 */
} else if ((pIn1->flags & MEM_Bool) != 0) {
c = pOp->opcode==OP_IfNot ? ! pIn1->u.b : pIn1->u.b;
} else {
- if (pOp->p5 == OPFLAG_BOOLREQ) {
- diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
- sql_value_text(pIn1), "boolean");
- rc = SQL_TARANTOOL_ERROR;
- goto abort_due_to_error;
- }
- double v;
- if (sqlVdbeRealValue(pIn1, &v) != 0) {
- diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
- sql_value_text(pIn1), "real");
- rc = SQL_TARANTOOL_ERROR;
- goto abort_due_to_error;
- }
- c = v != 0;
- if (pOp->opcode==OP_IfNot) c = !c;
+ diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+ sql_value_text(pIn1), "boolean");
+ rc = SQL_TARANTOOL_ERROR;
+ goto abort_due_to_error;
}
VdbeBranchTaken(c!=0, 2);
if (c) {
@@ -5301,7 +5290,7 @@ case OP_AggStep: {
if (pCtx->skipFlag) {
assert(pOp[-1].opcode==OP_CollSeq);
i = pOp[-1].p1;
- if (i) sqlVdbeMemSetInt64(&aMem[i], 1);
+ if (i) mem_set_bool(&aMem[i], true);
}
break;
}
diff --git a/test/sql-tap/cse.test.lua b/test/sql-tap/cse.test.lua
index 39c1cc4ca..78d0d2046 100755
--- a/test/sql-tap/cse.test.lua
+++ b/test/sql-tap/cse.test.lua
@@ -102,7 +102,7 @@ test:do_execsql_test(
test:do_execsql_test(
"cse-1.6.3",
[[
- SELECT CASE WHEN b THEN d WHEN e THEN f ELSE 999 END, b, c, d FROM t1
+ SELECT CASE WHEN CAST(b AS BOOLEAN) THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END, b, c, d FROM t1
]], {
-- <cse-1.6.3>
13, 11, 12, 13, 23, 21, 22, 23
@@ -112,7 +112,7 @@ test:do_execsql_test(
test:do_execsql_test(
"cse-1.6.4",
[[
- SELECT b, c, d, CASE WHEN b THEN d WHEN e THEN f ELSE 999 END FROM t1
+ SELECT b, c, d, CASE WHEN CAST(b AS BOOLEAN) THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END FROM t1
]], {
-- <cse-1.6.4>
11, 12, 13, 13, 21, 22, 23, 23
@@ -122,7 +122,7 @@ test:do_execsql_test(
test:do_execsql_test(
"cse-1.6.5",
[[
- SELECT b, c, d, CASE WHEN 0 THEN d WHEN e THEN f ELSE 999 END FROM t1
+ SELECT b, c, d, CASE WHEN false THEN d WHEN CAST(e AS BOOLEAN) THEN f ELSE 999 END FROM t1
]], {
-- <cse-1.6.5>
11, 12, 13, 15, 21, 22, 23, 25
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 9e9c4c551..c4724e636 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -703,7 +703,7 @@ test:do_execsql_test(
test:do_execsql_test(
"e_select-3.2.1b",
[[
- SELECT k FROM x1 LEFT JOIN x2 USING(k) WHERE x2.k
+ SELECT k FROM x1 LEFT JOIN x2 USING(k) WHERE x2.k <> 0
]], {
-- <e_select-3.2.1b>
1, 3, 5
diff --git a/test/sql-tap/orderby1.test.lua b/test/sql-tap/orderby1.test.lua
index ea03c494b..51e8d301f 100755
--- a/test/sql-tap/orderby1.test.lua
+++ b/test/sql-tap/orderby1.test.lua
@@ -735,7 +735,7 @@ test:do_execsql_test(
SELECT (
SELECT 'hardware' FROM (
SELECT 'software' ORDER BY 'firmware' ASC, 'sportswear' DESC
- ) GROUP BY 1 HAVING length(b)
+ ) GROUP BY 1 HAVING length(b) <> 0
)
FROM abc;
]], {
diff --git a/test/sql-tap/select1.test.lua b/test/sql-tap/select1.test.lua
index 1bad7679f..6beeb34cb 100755
--- a/test/sql-tap/select1.test.lua
+++ b/test/sql-tap/select1.test.lua
@@ -1492,13 +1492,13 @@ test:do_catchsql_test(
-- </select1-7.9>
})
-test:do_execsql_test(
+test:do_catchsql_test(
"select1-8.1",
[[
SELECT f1 FROM test1 WHERE 4.3+2.4 OR 1 ORDER BY f1
]], {
-- <select1-8.1>
- 11, 33
+ 1, 'Type mismatch: can not convert 6.7 to boolean'
-- </select1-8.1>
})
diff --git a/test/sql-tap/select2.test.lua b/test/sql-tap/select2.test.lua
index 7f0188d6f..e08c8b3b6 100755
--- a/test/sql-tap/select2.test.lua
+++ b/test/sql-tap/select2.test.lua
@@ -222,7 +222,7 @@ test:do_execsql_test(
test:do_execsql_test(
"select2-4.3",
[[
- SELECT * FROM aa CROSS JOIN bb WHERE NOT b;
+ SELECT * FROM aa CROSS JOIN bb WHERE NOT b <> 0;
]], {
-- <select2-4.3>
1, 0, 3, 0
@@ -242,7 +242,7 @@ test:do_execsql_test(
test:do_execsql_test(
"select2-4.5",
[[
- SELECT * FROM aa, bb WHERE NOT min(a,b);
+ SELECT * FROM aa, bb WHERE NOT min(a,b) <> 0;
]], {
More information about the Tarantool-patches
mailing list