[Tarantool-patches] [PATCH v1 4/7] sql: remove unnecessary calls of OP_ApplyType

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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
---
 src/box/sql/vdbe.c                   |  20 +--
 src/box/sql/wherecode.c              | 202 +--------------------------
 test/sql-tap/cast.test.lua           |  14 +-
 test/sql-tap/in4.test.lua            |   4 +-
 test/sql-tap/join.test.lua           |   4 +-
 test/sql-tap/tkt-9a8b09f8e6.test.lua |  22 +--
 6 files changed, 24 insertions(+), 242 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a69402720..c24265a1d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2506,7 +2506,7 @@ case OP_Close: {
 	break;
 }
 
-/* Opcode: SeekLT P1 P2 P3 P4 P5
+/* Opcode: SeekLT P1 P2 P3 P4 *
  * Synopsis: key=r[P3@P4]
  *
  * If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2522,8 +2522,6 @@ 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
  */
 case OP_SeekLT: {       /* jump, in3 */
@@ -2582,7 +2580,7 @@ case OP_SeekLT: {       /* jump, in3 */
 	break;
 }
 
-/* Opcode: SeekLE P1 P2 P3 P4 P5
+/* Opcode: SeekLE P1 P2 P3 P4 *
  * Synopsis: key=r[P3@P4]
  *
  * If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2605,8 +2603,6 @@ case OP_SeekLT: {       /* 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
  */
 case OP_SeekLE: {       /* jump, in3 */
@@ -2672,7 +2668,7 @@ case OP_SeekLE: {       /* jump, in3 */
 	break;
 }
 
-/* Opcode: SeekGE P1 P2 P3 P4 P5
+/* Opcode: SeekGE P1 P2 P3 P4 *
  * Synopsis: key=r[P3@P4]
  *
  * If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2695,11 +2691,6 @@ case OP_SeekLE: {       /* 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_SeekGE: {       /* jump, in3 */
@@ -2765,7 +2756,7 @@ case OP_SeekGE: {       /* jump, in3 */
 	break;
 }
 
-/* Opcode: SeekGT P1 P2 P3 P4 P5
+/* Opcode: SeekGT P1 P2 P3 P4 *
  * Synopsis: key=r[P3@P4]
  *
  * If cursor P1 refers to an SQL table (B-Tree that uses integer keys),
@@ -2781,9 +2772,6 @@ case OP_SeekGE: {       /* 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.
- *
  * P5 has the same meaning as for SeekGE.
  */
 case OP_SeekGT: {       /* 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,
     [[
-- 
2.25.1