[Tarantool-patches] [PATCH v1 4/7] sql: remove unnecessary calls of OP_ApplyType
Mergen Imeev
imeevma at tarantool.org
Fri Aug 6 02:41:52 MSK 2021
Thank you for ther review! My answer and new patch below.
On Thu, Aug 05, 2021 at 12:26:25AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> > index 96bcab110..92d374200 100644
> > --- a/src/box/sql/wherecode.c
> > +++ b/src/box/sql/wherecode.c
>
> <...>
>
> > -/**
> > - * Expression @rhs, which is the RHS of a comparison operation, is
> > - * either a vector of n elements or, if n==1, a scalar expression.
> > - * Before the comparison operation, types @types are to be applied
> > - * to the @rhs values. This function modifies entries within the
> > - * field sequence to SCALAR if either:
> > - *
> > - * * the comparison will be performed with no type, or
> > - * * the type change in @types is guaranteed not to change the value.
> > - */
> > -static void
> > -expr_cmp_update_rhs_type(struct Expr *rhs, int n, enum field_type *types)
> > -{
> > - for (int i = 0; i < n; i++) {
> > - Expr *p = sqlVectorFieldSubexpr(rhs, i);
> > - enum field_type expr_type = sql_expr_type(p);
> > - if (sql_type_result(expr_type, types[i]) == FIELD_TYPE_SCALAR ||
> > - sql_expr_needs_no_type_change(p, types[i])) {
>
> sql_expr_needs_no_type_change is now unused.
Thanks, fixed.
New patch:
commit ecbc9842d5641e6746d909fe0c16fa6120b21e09
Author: Mergen Imeev <imeevma at gmail.com>
Date: Tue Jul 27 22:51:34 2021 +0300
sql: remove unnecessary calls of OP_ApplyType
Since the OP_Seek* opcodes now work using the new implicit casting
rules, we don't need to call OP_ApplyType before them. This patch
removes such calls.
Part of #4230
Part of #4470
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index d2624516c..b9fc5bfc5 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2246,36 +2246,6 @@ sqlExprCanBeNull(const Expr * p)
}
}
-bool
-sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
-{
- u8 op;
- if (type == FIELD_TYPE_SCALAR)
- return true;
- while (p->op == TK_UPLUS || p->op == TK_UMINUS) {
- p = p->pLeft;
- }
- op = p->op;
- if (op == TK_REGISTER)
- op = p->op2;
- switch (op) {
- case TK_INTEGER:
- return type == FIELD_TYPE_INTEGER;
- case TK_FLOAT:
- return type == FIELD_TYPE_DOUBLE;
- case TK_STRING:
- return type == FIELD_TYPE_STRING;
- case TK_BLOB:
- return type == FIELD_TYPE_VARBINARY;
- case TK_COLUMN_REF:
- /* p cannot be part of a CHECK constraint. */
- assert(p->iTable >= 0);
- return p->iColumn < 0 && sql_type_is_numeric(type);
- default:
- return false;
- }
-}
-
/*
* pX is the RHS of an IN operator. If pX is a SELECT statement
* that can be simplified to a direct table access, then return
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e2e550978..6b3444cf3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3182,19 +3182,6 @@ int sqlExprIsTableConstant(Expr *, int);
int sqlExprIsInteger(Expr *, int *);
int sqlExprCanBeNull(const Expr *);
-/**
- * Return TRUE if the given expression is a constant which would
- * be unchanged by OP_ApplyType with the type given in the second
- * argument.
- *
- * This routine is used to determine if the OP_ApplyType operation
- * can be omitted. When in doubt return FALSE. A false negative
- * is harmless. A false positive, however, can result in the wrong
- * answer.
- */
-bool
-sql_expr_needs_no_type_change(const struct Expr *expr, enum field_type type);
-
/**
* This routine generates VDBE code that causes a single row of a
* single table to be deleted. Both the original table entry and
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 371be205e..aedff8b09 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2504,7 +2504,7 @@ case OP_Close: {
break;
}
-/* Opcode: SeekLT P1 P2 P3 P4 P5
+/* Opcode: SeekLT P1 P2 P3 P4 *
* Synopsis: key=r[P3 at P4]
*
* If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2520,11 +2520,9 @@ case OP_Close: {
* from the end toward the beginning. In other words, the cursor is
* configured to use Prev, not Next.
*
- * P5 has the same meaning as for SeekGE.
- *
* See also: Found, NotFound, SeekGt, SeekGe, SeekLe
*/
-/* Opcode: SeekGT P1 P2 P3 P4 P5
+/* Opcode: SeekGT P1 P2 P3 P4 *
* Synopsis: key=r[P3 at P4]
*
* If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2539,11 +2537,6 @@ case OP_Close: {
* This opcode leaves the cursor configured to move in forward order,
* from the beginning toward the end. In other words, the cursor is
* configured to use Next, not Prev.
- *
- * If P5 is not zero, than it is offset of integer fields in input
- * vector. Force corresponding value to be INTEGER.
- *
- * P5 has the same meaning as for SeekGE.
*/
case OP_SeekLT: /* jump, in3 */
case OP_SeekGT: { /* jump, in3 */
@@ -2592,7 +2585,7 @@ case OP_SeekGT: { /* jump, in3 */
break;
}
-/* Opcode: SeekLE P1 P2 P3 P4 P5
+/* Opcode: SeekLE P1 P2 P3 P4 *
* Synopsis: key=r[P3 at P4]
*
* If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2615,11 +2608,9 @@ case OP_SeekGT: { /* jump, in3 */
* The IdxGE opcode will be skipped if this opcode succeeds, but the
* IdxGE opcode will be used on subsequent loop iterations.
*
- * P5 has the same meaning as for SeekGE.
- *
* See also: Found, NotFound, SeekGt, SeekGe, SeekLt
*/
-/* Opcode: SeekGE P1 P2 P3 P4 P5
+/* Opcode: SeekGE P1 P2 P3 P4 *
* Synopsis: key=r[P3 at P4]
*
* If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2642,11 +2633,6 @@ case OP_SeekGT: { /* jump, in3 */
* from the beginning toward the end. In other words, the cursor is
* configured to use Next, not Prev.
*
- * If P5 is not zero, than it is offset of integer fields in input
- * vector. Force corresponding value to be INTEGER, in case it
- * is floating point value. Alongside with that, type of
- * iterator may be changed: a > 1.5 -> a >= 2.
- *
* See also: Found, NotFound, SeekLt, SeekGt, SeekLe
*/
case OP_SeekLE: /* jump, in3 */
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 96bcab110..92d374200 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -335,72 +335,6 @@ disableTerm(WhereLevel * pLevel, WhereTerm * pTerm)
}
}
-/**
- * Code an OP_ApplyType opcode to apply the column type string
- * @types to the n registers starting at @base.
- *
- * As an optimization, SCALAR entries (which are no-ops) at the
- * beginning and end of @types are ignored. If all entries in
- * @types are SCALAR, then no code gets generated.
- *
- * This routine makes its own copy of @types so that the caller is
- * free to modify @types after this routine returns.
- */
-static void
-emit_apply_type(Parse *pParse, int base, int n, enum field_type *types)
-{
- Vdbe *v = pParse->pVdbe;
- if (types == NULL) {
- assert(pParse->db->mallocFailed);
- return;
- }
- assert(v != 0);
-
- /*
- * Adjust base and n to skip over SCALAR entries at the
- * beginning and end of the type sequence.
- */
- while (n > 0 && types[0] == FIELD_TYPE_SCALAR) {
- n--;
- base++;
- types++;
- }
- while (n > 1 && types[n - 1] == FIELD_TYPE_SCALAR) {
- n--;
- }
-
- if (n > 0) {
- enum field_type *types_dup = field_type_sequence_dup(pParse,
- types, n);
- sqlVdbeAddOp4(v, OP_ApplyType, base, n, 0,
- (char *) types_dup, P4_DYNAMIC);
- sql_expr_type_cache_change(pParse, base, n);
- }
-}
-
-/**
- * Expression @rhs, which is the RHS of a comparison operation, is
- * either a vector of n elements or, if n==1, a scalar expression.
- * Before the comparison operation, types @types are to be applied
- * to the @rhs values. This function modifies entries within the
- * field sequence to SCALAR if either:
- *
- * * the comparison will be performed with no type, or
- * * the type change in @types is guaranteed not to change the value.
- */
-static void
-expr_cmp_update_rhs_type(struct Expr *rhs, int n, enum field_type *types)
-{
- for (int i = 0; i < n; i++) {
- Expr *p = sqlVectorFieldSubexpr(rhs, i);
- enum field_type expr_type = sql_expr_type(p);
- if (sql_type_result(expr_type, types[i]) == FIELD_TYPE_SCALAR ||
- sql_expr_needs_no_type_change(p, types[i])) {
- types[i] = FIELD_TYPE_SCALAR;
- }
- }
-}
-
/*
* Generate code for a single equality term of the WHERE clause. An equality
* term can be either X=expr or X IN (...). pTerm is the term to be
@@ -644,8 +578,7 @@ static int
codeAllEqualityTerms(Parse * pParse, /* Parsing context */
WhereLevel * pLevel, /* Which nested loop of the FROM we are coding */
int bRev, /* Reverse the order of IN operators */
- int nExtraReg, /* Number of extra registers to allocate */
- enum field_type **res_type)
+ int nExtraReg) /* Number of extra registers to allocate */
{
u16 nEq; /* The number of == or IN constraints to code */
u16 nSkip; /* Number of left-most columns to skip */
@@ -733,7 +666,6 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */
}
}
}
- *res_type = type;
return regBase;
}
@@ -905,16 +837,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
int iIdxCur; /* The VDBE cursor for the index */
int nExtraReg = 0; /* Number of extra registers needed */
int op; /* Instruction opcode */
- /* Types for start of range constraint. */
- enum field_type *start_types;
- /* Types for end of range constraint */
- enum field_type *end_types = NULL;
u8 bSeekPastNull = 0; /* True to seek past initial nulls */
u8 bStopAtNull = 0; /* Add condition to terminate at NULLs */
- int force_integer_reg = -1; /* If non-negative: number of
- * column which must be converted
- * to integer type, used for IPK.
- */
struct index_def *idx_def = pLoop->index_def;
assert(idx_def != NULL);
@@ -996,16 +920,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
* starting at regBase.
*/
regBase =
- codeAllEqualityTerms(pParse, pLevel, bRev, nExtraReg,
- &start_types);
- if (start_types != NULL && nTop) {
- uint32_t len = 0;
- for (enum field_type *tmp = &start_types[nEq];
- *tmp != field_type_MAX; tmp++, len++);
- uint32_t sz = len * sizeof(enum field_type);
- end_types = sqlDbMallocRaw(db, sz);
- memcpy(end_types, &start_types[nEq], sz);
- }
+ codeAllEqualityTerms(pParse, pLevel, bRev, nExtraReg);
addrNxt = pLevel->addrNxt;
testcase(pRangeStart && (pRangeStart->eOperator & WO_LE) != 0);
@@ -1030,10 +945,6 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
VdbeCoverage(v);
}
- if (start_types) {
- expr_cmp_update_rhs_type(pRight, nBtm,
- &start_types[nEq]);
- }
nConstraint += nBtm;
testcase(pRangeStart->wtFlags & TERM_VIRTUAL);
if (sqlExprIsVector(pRight) == 0) {
@@ -1048,94 +959,6 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
startEq = 0;
start_constraints = 1;
}
- /*
- * Tarantool's iterator over integer fields doesn't
- * tolerate floating point values. Hence, if term
- * is equality comparison and value of operand is
- * not integer, we can skip it since it always
- * results in false: INT a == 0.5 -> false;
- * It is done using OP_MustBeInt facilities.
- * In case term is greater comparison (a > ?), we
- * should notify OP_SeekGT to process truncation of
- * floating point value: a > 0.5 -> a >= 1;
- * It is done by setting P5 flag for OP_Seek*.
- * It is worth mentioning that we do not need
- * this step when it comes for less (<) comparison
- * of nullable field. Key is NULL in this case:
- * values are ordered as NULL, ... NULL, min_value,
- * so to fetch min value we pass NULL to GT iterator.
- * The only exception is less comparison in
- * conjunction with ORDER BY DESC clause:
- * in such situation we use LE iterator and
- * truncated value to compare. But then
- * pRangeStart == NULL.
- * This procedure is correct for compound index:
- * only one comparison of less/greater type can be
- * used at the same time. For instance,
- * a < 1.5 AND b > 0.5 is handled by SeekGT using
- * column a and fetching column b from tuple and
- * OP_Le comparison.
- *
- * Note that OP_ApplyType, which is emitted before
- * OP_Seek** doesn't truncate floating point to
- * integer. That's why we need this routine.
- * Also, note that terms are separated by OR
- * predicates, so we consider term as sequence
- * of AND'ed predicates.
- */
- size_t addrs_sz;
- int *seek_addrs = region_alloc_array(&pParse->region,
- typeof(seek_addrs[0]), nEq,
- &addrs_sz);
- if (seek_addrs == NULL) {
- diag_set(OutOfMemory, addrs_sz, "region_alloc_array",
- "seek_addrs");
- pParse->is_aborted = true;
- return 0;
- }
- memset(seek_addrs, 0, addrs_sz);
- for (int i = 0; i < nEq; i++) {
- enum field_type type = idx_def->key_def->parts[i].type;
- if (type == FIELD_TYPE_INTEGER ||
- type == FIELD_TYPE_UNSIGNED) {
- /*
- * OP_MustBeInt consider NULLs as
- * non-integer values, so firstly
- * check whether value is NULL or not.
- */
- seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
- regBase);
- sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
- addrNxt);
- start_types[i] = FIELD_TYPE_SCALAR;
- /*
- * We need to notify column cache
- * that type of value may change
- * so we should fetch value from
- * tuple again rather then copy
- * from register.
- */
- sql_expr_type_cache_change(pParse, regBase + i,
- 1);
- }
- }
- /* Inequality constraint comes always at the end of list. */
- part_count = idx_def->key_def->part_count;
- if (pRangeStart != NULL) {
- /*
- * nEq == 0 means that filter condition
- * contains only inequality.
- */
- uint32_t ineq_idx = nEq == 0 ? 0 : nEq - 1;
- assert(ineq_idx < part_count);
- enum field_type ineq_type =
- idx_def->key_def->parts[ineq_idx].type;
- if (ineq_type == FIELD_TYPE_INTEGER ||
- ineq_type == FIELD_TYPE_UNSIGNED)
- force_integer_reg = regBase + nEq;
- }
- emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
- start_types);
if (pLoop->nSkip > 0 && nConstraint == pLoop->nSkip) {
/* The skip-scan logic inside the call to codeAllEqualityConstraints()
* above has already left the cursor sitting on the correct row,
@@ -1145,20 +968,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
op = aStartOp[(start_constraints << 2) +
(startEq << 1) + bRev];
assert(op != 0);
- for (uint32_t i = 0; i < nEq; ++i) {
- if (seek_addrs[i] != 0)
- sqlVdbeJumpHere(v, seek_addrs[i]);
- }
sqlVdbeAddOp4Int(v, op, iIdxCur, addrNxt, regBase,
nConstraint);
- /* If this is Seek* opcode, and IPK is detected in the
- * constraints vector: force it to be integer.
- */
- if ((op == OP_SeekGE || op == OP_SeekGT
- || op == OP_SeekLE || op == OP_SeekLT)
- && force_integer_reg > 0) {
- sqlVdbeChangeP5(v, force_integer_reg);
- }
VdbeCoverage(v);
VdbeCoverageIf(v, op == OP_Rewind);
testcase(op == OP_Rewind);
@@ -1188,13 +999,6 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
addrNxt);
VdbeCoverage(v);
}
- if (end_types) {
- expr_cmp_update_rhs_type(pRight, nTop, end_types);
- emit_apply_type(pParse, regBase + nEq, nTop,
- end_types);
- } else {
- assert(pParse->db->mallocFailed);
- }
nConstraint += nTop;
testcase(pRangeEnd->wtFlags & TERM_VIRTUAL);
@@ -1208,8 +1012,6 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W
endEq = 0;
nConstraint++;
}
- sqlDbFree(db, start_types);
- sqlDbFree(db, end_types);
/* Top of the loop body */
pLevel->p2 = sqlVdbeCurrentAddr(v);
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index b8ad23317..b570fc878 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
local test = require("sqltester")
-test:plan(107)
+test:plan(108)
--!./tcltestrunner.lua
-- 2005 June 25
@@ -1144,4 +1144,16 @@ test:do_catchsql_test(
1, "Type mismatch: can not convert double(2.5) to string"
})
+-- Make sure that search using index in field type number work right.
+test:do_execsql_test(
+ "cast-11",
+ [[
+ CREATE TABLE t6(d DOUBLE PRIMARY KEY);
+ INSERT INTO t6 VALUES(10000000000000000);
+ SELECT d FROM t6 WHERE d < 10000000000000001 and d > 9999999999999999;
+ DROP TABLE t6;
+ ]], {
+ 10000000000000000
+ })
+
test:finish_test()
diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
index 8442944b9..aa6483697 100755
--- a/test/sql-tap/in4.test.lua
+++ b/test/sql-tap/in4.test.lua
@@ -147,13 +147,13 @@ test:do_execsql_test(
-- </in4-2.7>
})
-test:do_execsql_test(
+test:do_catchsql_test(
"in4-2.8",
[[
SELECT b FROM t2 WHERE a IN ('', '0.0.0', '2')
]], {
-- <in4-2.8>
- "two"
+ 1, "Type mismatch: can not convert string('') to integer"
-- </in4-2.8>
})
diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua
index 48639a7b3..abfabfacf 100755
--- a/test/sql-tap/join.test.lua
+++ b/test/sql-tap/join.test.lua
@@ -1028,13 +1028,13 @@ test:do_test(
-- </join-11.8>
})
-test:do_execsql_test(
+test:do_catchsql_test(
"join-11.9",
[[
SELECT * FROM t1 NATURAL JOIN t2
]], {
-- <join-11.9>
- "one", "1", "two", "2"
+ 1, "Type mismatch: can not convert string('1') to integer"
-- </join-11.9>
})
diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
index 43322468d..cc321c2f6 100755
--- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
+++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
local test = require("sqltester")
-test:plan(47)
+test:plan(45)
--!./tcltestrunner.lua
-- 2014 June 26
@@ -179,16 +179,6 @@ test:do_execsql_test(
-- </3.2>
})
-test:do_execsql_test(
- 3.3,
- [[
- SELECT x FROM t2 WHERE x IN ('1');
- ]], {
- -- <3.3>
- 1
- -- </3.3>
- })
-
test:do_execsql_test(
3.5,
[[
@@ -209,16 +199,6 @@ test:do_execsql_test(
-- </3.6>
})
-test:do_execsql_test(
- 3.7,
- [[
- SELECT x FROM t2 WHERE '1' IN (x);
- ]], {
- -- <3.7>
- 1
- -- </3.7>
- })
-
test:do_execsql_test(
4.1,
[[
More information about the Tarantool-patches
mailing list