[tarantool-patches] [PATCH 0/3] Fix passing key with different from iterator's type

[tarantool-patches] [PATCH 0/3] Fix passing key with different from iterator's type

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/fix-bool-fp-to-int
Issue: https://github.com/tarantool/tarantool/issues/4187

This patch-set fixes two bugs connected with passing to Tarantool
iterators values of different from covered by iterator fields types.

Nikita Pettik (3):
  sql: remove redundant check of space format from QP
  sql: remove redundant type derivation from QP
  sql: fix passing  FP values to integer iterator

 src/box/sql/vdbe.c      | 23 +++++++-----
 src/box/sql/wherecode.c | 96 +++++++++++++++++++++++++++++--------------------
 test/sql/types.result   | 70 ++++++++++++++++++++++++++++++++++++
 test/sql/types.test.lua | 21 +++++++++++
 4 files changed, 163 insertions(+), 47 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/3] sql: remove redundant check of space format from QP

[Nikita Pettik]

In SQL we are able to execute queries involving spaces only with formats.
Otherwise, at the very beginning of query compilation error is raised.
So, after that any checks of format existence are redundant.
---
 src/box/sql/wherecode.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index a453fe979..6f72506ad 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -967,7 +967,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 		assert(idx_def != NULL);
 		struct space *space = space_by_id(idx_def->space_id);
 		assert(space != NULL);
-		bool is_format_set = space->def->field_count != 0;
+		assert(space->def->field_count != 0);
 		iIdxCur = pLevel->iIdxCur;
 		assert(nEq >= pLoop->nSkip);
 
@@ -996,8 +996,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 			 * FYI: entries in an index are ordered as follows:
 			 *      NULL, ... NULL, min_value, ...
 			 */
-			if (is_format_set &&
-			    space->def->fields[j].is_nullable) {
+			if (space->def->fields[j].is_nullable) {
 				assert(pLoop->nSkip == 0);
 				bSeekPastNull = 1;
 				nExtraReg = 1;
@@ -1020,8 +1019,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 			nExtraReg = MAX(nExtraReg, pLoop->nTop);
 			if (pRangeStart == 0) {
 				j = idx_def->key_def->parts[nEq].fieldno;
-				if (is_format_set &&
-				    space->def->fields[j].is_nullable)
+				if (space->def->fields[j].is_nullable)
 					bSeekPastNull = 1;
 			}
 		}
@@ -1099,12 +1097,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 		}
 		struct index_def *idx_pk = space->index[0]->def;
 		int fieldno = idx_pk->key_def->parts[0].fieldno;
-		enum field_type fd_type = is_format_set ?
-					  space->def->fields[fieldno].type :
-					  FIELD_TYPE_SCALAR;
 
 		uint32_t pk_part_count = idx_pk->key_def->part_count;
-		if (pk_part_count == 1 && fd_type == FIELD_TYPE_INTEGER) {
+		if (pk_part_count == 1 &&
+		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
 			/* Right now INTEGER PRIMARY KEY is the only option to
 			 * get Tarantool's INTEGER column type. Need special handling
 			 * here: try to loosely convert FLOAT to INT. If RHS type
-- 
2.15.1

[tarantool-patches] [PATCH 2/3] sql: remove redundant type derivation from QP

[Nikita Pettik]

Before value to be scanned in index search is passed to the iterator, it
is subjected to implicit type casting (which is implemented by
OP_ApplyType). If value can't be converted to required type,
user-friendly message is raised. Without this cast, type of iterator may
not match with type of key which in turn results in unexpected error.
However, array of types which is used to provide type conversions is
different from types of indexed fields: it is  modified depending on
types of comparison's operands. For instance, when boolean field is
compared with blob value, resulting type is assumed to be scalar. In
turn, conversion to scalar is no-op.  As a result, value with MP_BIN
format is passed to the iterator over boolean field.  To fix that let's
remove this transformation of types. Moreover, it seems to be redundant.

Part of #4187
---
 src/box/sql/wherecode.c | 10 ----------
 test/sql/types.result   | 30 ++++++++++++++++++++++++++++++
 test/sql/types.test.lua | 12 ++++++++++++
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 6f72506ad..977c0fced 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -769,16 +769,6 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
 						  pLevel->addrBrk);
 				VdbeCoverage(v);
 			}
-			if (type != NULL) {
-				enum field_type rhs_type =
-					sql_expr_type(pRight);
-				if (sql_type_result(rhs_type, type[j]) ==
-				    FIELD_TYPE_SCALAR) {
-					type[j] = FIELD_TYPE_SCALAR;
-				}
-				if (sql_expr_needs_no_type_change(pRight, type[j]))
-					type[j] = FIELD_TYPE_SCALAR;
-			}
 		}
 	}
 	*res_type = type;
diff --git a/test/sql/types.result b/test/sql/types.result
index bc4518c01..758018eb5 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -897,3 +897,33 @@ box.execute('SELECT ?', {true})
   rows:
   - [true]
 ...
+-- gh-4187: make sure that value passsed to the iterator has
+-- the same type as indexed fields.
+--
+box.execute("CREATE TABLE tboolean (s1 BOOLEAN PRIMARY KEY);")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO tboolean VALUES (TRUE);")
+---
+- row_count: 1
+...
+box.execute("SELECT * FROM tboolean WHERE s1 = x'44';")
+---
+- error: 'Type mismatch: can not convert D to boolean'
+...
+box.execute("SELECT * FROM tboolean WHERE s1 = 'abc';")
+---
+- error: 'Type mismatch: can not convert abc to boolean'
+...
+box.execute("SELECT * FROM tboolean WHERE s1 = 1;")
+---
+- error: 'Type mismatch: can not convert INTEGER to boolean'
+...
+box.execute("SELECT * FROM tboolean WHERE s1 = 1.123;")
+---
+- error: 'Type mismatch: can not convert REAL to boolean'
+...
+box.space.TBOOLEAN:drop()
+---
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index c51660cb9..3c7bd5ea3 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -216,3 +216,15 @@ box.execute('SELECT \'9223372036854\' + 1;')
 
 -- Fix BOOLEAN bindings.
 box.execute('SELECT ?', {true})
+
+-- gh-4187: make sure that value passsed to the iterator has
+-- the same type as indexed fields.
+--
+box.execute("CREATE TABLE tboolean (s1 BOOLEAN PRIMARY KEY);")
+box.execute("INSERT INTO tboolean VALUES (TRUE);")
+box.execute("SELECT * FROM tboolean WHERE s1 = x'44';")
+box.execute("SELECT * FROM tboolean WHERE s1 = 'abc';")
+box.execute("SELECT * FROM tboolean WHERE s1 = 1;")
+box.execute("SELECT * FROM tboolean WHERE s1 = 1.123;")
+
+box.space.TBOOLEAN:drop()
-- 
2.15.1

[tarantool-patches] Re: [PATCH 2/3] sql: remove redundant type derivation from QP

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 6f72506ad..977c0fced 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -769,16 +769,6 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
>  						  pLevel->addrBrk);
>  				VdbeCoverage(v);
>  			}
> -			if (type != NULL) {
> -				enum field_type rhs_type =
> -					sql_expr_type(pRight);
> -				if (sql_type_result(rhs_type, type[j]) ==
> -				    FIELD_TYPE_SCALAR) {
> -					type[j] = FIELD_TYPE_SCALAR;
> -				}
> -				if (sql_expr_needs_no_type_change(pRight, type[j]))
> -					type[j] = FIELD_TYPE_SCALAR;
> -			}
>  		}

There is a comment on that function:

>  * Before returning, @types is set to point to a buffer containing a
>  * copy of the column types array of the index allocated using
>  * sqlDbMalloc(). Except, entries in the copy of the string associated
>  * with equality constraints that use SCALAR type are set to
>  * SCALAR. This is to deal with SQL such as the following:
>  *
>  *   CREATE TABLE t1(a TEXT PRIMARY KEY, b BLOB);
>  *   SELECT ... FROM t1 AS t2, t1 WHERE t1.a = t2.b;
>  *
>  * In the example above, the index on t1(a) has STRING type. But since
>  * the right hand side of the equality constraint (t2.b) has SCALAR type,
>  * no conversion should be attempted before using a t2.b value as part of
>  * a key to search the index. Hence the first byte in the returned type
>  * string in this example would be set to SCALAR.

Looks outdated, especially after your change. Now we do not
convert to scalars. Even before your patch it was outdated, as I
understand. Lets fix it alongside, since now it makes no sense at
all.

Also I need you comment on that code, which is above the hunk
you deleted:

> 		if (pTerm->eOperator & WO_IN) {
> 			if (pTerm->pExpr->flags & EP_xIsSelect) {
> 				/* No type ever needs to be (or should be) applied to a value
> 				 * from the RHS of an "? IN (SELECT ...)" expression. The
> 				 * sqlFindInIndex() routine has already ensured that the
> 				 * type of the comparison has been applied to the value.
> 				 */
> 				if (type != NULL)
> 					type[j] = FIELD_TYPE_SCALAR;
> 			}

Looks, like this code still thinks, that we put into IN operator
values of any mixed types, but I thought, that we want to forbid
that. That IN should have only values of the same type. Isn't it
a bug?

[tarantool-patches] Re: [PATCH 2/3] sql: remove redundant type derivation from QP

[n.pettik]

> On 28 May 2019, at 00:49, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index 6f72506ad..977c0fced 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -769,16 +769,6 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
>> 						  pLevel->addrBrk);
>> 				VdbeCoverage(v);
>> 			}
>> -			if (type != NULL) {
>> -				enum field_type rhs_type =
>> -					sql_expr_type(pRight);
>> -				if (sql_type_result(rhs_type, type[j]) ==
>> -				    FIELD_TYPE_SCALAR) {
>> -					type[j] = FIELD_TYPE_SCALAR;
>> -				}
>> -				if (sql_expr_needs_no_type_change(pRight, type[j]))
>> -					type[j] = FIELD_TYPE_SCALAR;
>> -			}
>> 		}
> 
> There is a comment on that function:
> 
>> * Before returning, @types is set to point to a buffer containing a
>> * copy of the column types array of the index allocated using
>> * sqlDbMalloc(). Except, entries in the copy of the string associated
>> * with equality constraints that use SCALAR type are set to
>> * SCALAR. This is to deal with SQL such as the following:
>> *
>> *   CREATE TABLE t1(a TEXT PRIMARY KEY, b BLOB);
>> *   SELECT ... FROM t1 AS t2, t1 WHERE t1.a = t2.b;
>> *
>> * In the example above, the index on t1(a) has STRING type. But since
>> * the right hand side of the equality constraint (t2.b) has SCALAR type,
>> * no conversion should be attempted before using a t2.b value as part of
>> * a key to search the index. Hence the first byte in the returned type
>> * string in this example would be set to SCALAR.
> 
> Looks outdated, especially after your change. Now we do not
> convert to scalars. Even before your patch it was outdated, as I
> understand. Lets fix it alongside, since now it makes no sense at
> all.

Ok:

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 977c0fced..d776c8ade 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -665,18 +665,8 @@ codeEqualityTerm(Parse * pParse,   /* The parsing context */
  *
  * Before returning, @types is set to point to a buffer containing a
  * copy of the column types array of the index allocated using
- * sqlDbMalloc(). Except, entries in the copy of the string associated
- * with equality constraints that use SCALAR type are set to
- * SCALAR. This is to deal with SQL such as the following:
- *
- *   CREATE TABLE t1(a TEXT PRIMARY KEY, b BLOB);
- *   SELECT ... FROM t1 AS t2, t1 WHERE t1.a = t2.b;
- *
- * In the example above, the index on t1(a) has STRING type. But since
- * the right hand side of the equality constraint (t2.b) has SCALAR type,
- * no conversion should be attempted before using a t2.b value as part of
- * a key to search the index. Hence the first byte in the returned type
- * string in this example would be set to SCALAR.
+ * sqlDbMalloc(). This array is passed to OP_ApplyType to provide
+ * correct implicit conversions.
  */
 static int
 codeAllEqualityTerms(Parse * pParse,   /* Parsing context */

> Also I need you comment on that code, which is above the hunk
> you deleted:
> 
>> 		if (pTerm->eOperator & WO_IN) {
>> 			if (pTerm->pExpr->flags & EP_xIsSelect) {
>> 				/* No type ever needs to be (or should be) applied to a value
>> 				 * from the RHS of an "? IN (SELECT ...)" expression. The
>> 				 * sqlFindInIndex() routine has already ensured that the
>> 				 * type of the comparison has been applied to the value.
>> 				 */
>> 				if (type != NULL)
>> 					type[j] = FIELD_TYPE_SCALAR;
>> 			}
> 
> Looks, like this code still thinks, that we put into IN operator
> values of any mixed types, but I thought, that we want to forbid
> that. That IN should have only values of the same type. Isn't it
> a bug?

Why do you think so? IN operator is decomposed into a series of
equality constraints, so it uses the same comparison rules as
other comparison operators. This code concerns pattern
? IN (SELECT …). In turn, in this situation index can be used only
if types of result columns are the same as ? column: it is verified
in sqlFindIndex:

/*
 * Index search is possible only if types
 * of columns match.
 */
if (idx_type != lhs_type)
       type_is_suitable = false;

Hence, we may skip implicit cast on these fields.

[tarantool-patches] [PATCH 3/3] sql: fix passing FP values to integer iterator

[Nikita Pettik]

Before this patch it was impossible to compare indexed field of integer
type and floating point value. For instance:

CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);
INSERT INTO t1 VALUES (1, 1);
SELECT * FROM t1 WHERE a = 1.5;
---
- error: 'Failed to execute SQL statement: Supplied key type of part 0 does not match
    index part type: expected integer'
...

That happened due to the fact that type casting mechanism (OP_ApplyType)
doesn't affect FP value when it is converted to integer. Hence, FP value
was passed to the iterator over integer field which resulted in error.
Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
with this problem for each equality comparison involving integer field
we emit OP_MustBeInt, which checks whether value to be compared is
integer or not. If the latter, we assume that result of comparison is
always false and continue processing query.  For inequality constraints
we pass auxiliary flag to OP_Seek** opcodes to notify it that one of key
fields must be truncated to integer (in case of FP value) alongside with
changing iterator's type: a > 1.5 -> a >= 2.

Closes #4187
---
 src/box/sql/vdbe.c      | 23 +++++++++------
 src/box/sql/wherecode.c | 76 +++++++++++++++++++++++++++++++++++--------------
 test/sql/types.result   | 40 ++++++++++++++++++++++++++
 test/sql/types.test.lua |  9 ++++++
 4 files changed, 118 insertions(+), 30 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3bd82234e..3ead6b40f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3478,7 +3478,6 @@ case OP_SeekGT: {       /* jump, in3 */
 	int nField;        /* Number of columns or fields in the key */
 	i64 iKey;          /* The id we are to seek to */
 	int eqOnly;        /* Only interested in == results */
-	int reg_ipk=0;     /* Register number which holds IPK. */
 
 	assert(pOp->p1>=0 && pOp->p1<p->nCursor);
 	assert(pOp->p2!=0);
@@ -3496,15 +3495,22 @@ case OP_SeekGT: {       /* jump, in3 */
 	pC->seekOp = pOp->opcode;
 #endif
 	iKey = 0;
-	reg_ipk = pOp->p5;
-
-	if (reg_ipk > 0) {
+	/*
+	 * In case floating value is intended to be passed to
+	 * iterator over integer field, we must truncate it to
+	 * integer value and change type of iterator:
+	 * a > 1.5 -> a >= 2
+	 */
+	int int_field = pOp->p5;
 
+	if (int_field > 0) {
 		/* The input value in P3 might be of any type: integer, real, string,
 		 * blob, or NULL.  But it needs to be an integer before we can do
 		 * the seek, so convert it.
 		 */
-		pIn3 = &aMem[reg_ipk];
+		pIn3 = &aMem[int_field];
+		if ((pIn3->flags & MEM_Null) != 0)
+			goto skip_truncate;
 		if ((pIn3->flags & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
 			mem_apply_numeric_type(pIn3);
 		}
@@ -3563,6 +3569,7 @@ case OP_SeekGT: {       /* jump, in3 */
 			}
 		}
 	}
+skip_truncate:
 	/*
 	 * For a cursor with the OPFLAG_SEEKEQ hint, only the
 	 * OP_SeekGE and OP_SeekLE opcodes are allowed, and these
@@ -3585,9 +3592,9 @@ case OP_SeekGT: {       /* jump, in3 */
 	r.key_def = pC->key_def;
 	r.nField = (u16)nField;
 
-	if (reg_ipk > 0) {
-		aMem[reg_ipk].u.i = iKey;
-		aMem[reg_ipk].flags = MEM_Int;
+	if (int_field > 0) {
+		aMem[int_field].u.i = iKey;
+		aMem[int_field].flags = MEM_Int;
 	}
 
 	r.default_rc = ((1 & (oc - OP_SeekLT)) ? -1 : +1);
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 977c0fced..e779729c7 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 			start_constraints = 1;
 		}
 		struct index_def *idx_pk = space->index[0]->def;
-		int fieldno = idx_pk->key_def->parts[0].fieldno;
-
 		uint32_t pk_part_count = idx_pk->key_def->part_count;
-		if (pk_part_count == 1 &&
-		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
-			/* Right now INTEGER PRIMARY KEY is the only option to
-			 * get Tarantool's INTEGER column type. Need special handling
-			 * here: try to loosely convert FLOAT to INT. If RHS type
-			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
-			 */
-			int limit = pRangeStart == NULL ? nEq : nEq + 1;
-			for (int i = 0; i < limit; i++) {
-				if (idx_def->key_def->parts[i].fieldno ==
-				    idx_pk->key_def->parts[0].fieldno) {
-					/* Here: we know for sure that table has INTEGER
-					   PRIMARY KEY, single column, and Index we're
-					   trying to use for scan contains this column. */
-					if (i < nEq)
-						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
-					else
-						force_integer_reg = regBase + i;
-					break;
-				}
+		/*
+		 * 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:
+		 * 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.
+		 */
+		int seek_addr = 0;
+		for (int i = 0; i < nEq; i++) {
+			enum field_type type = idx_def->key_def->parts[i].type;
+			if (type == FIELD_TYPE_INTEGER) {
+				/*
+				 * OP_MustBeInt consider NULLs as
+				 * non-integer values, so firstly
+				 * check whether value is NULL or not.
+				 */
+				seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
+							  regBase);
+				sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
+					      addrNxt);
 			}
 		}
+		/* Inequality constraint comes always at the end of list. */
+		if (pRangeStart != NULL &&
+		    idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
+			force_integer_reg = regBase + nEq;
 		emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
 				start_types);
 		if (pLoop->nSkip > 0 && nConstraint == pLoop->nSkip) {
@@ -1122,6 +1152,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 			op = aStartOp[(start_constraints << 2) +
 				      (startEq << 1) + bRev];
 			assert(op != 0);
+			if (seek_addr != 0)
+				sqlVdbeJumpHere(v, seek_addr);
 			sqlVdbeAddOp4Int(v, op, iIdxCur, addrNxt, regBase,
 					     nConstraint);
 			/* If this is Seek* opcode, and IPK is detected in the
diff --git a/test/sql/types.result b/test/sql/types.result
index 758018eb5..274d73dd2 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -927,3 +927,43 @@ box.execute("SELECT * FROM tboolean WHERE s1 = 1.123;")
 box.space.TBOOLEAN:drop()
 ---
 ...
+box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO t1 VALUES (1, 1);")
+---
+- row_count: 1
+...
+box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
+---
+- metadata:
+  - name: A
+    type: integer
+  rows: []
+...
+box.execute("SELECT a FROM t1 WHERE a = 1.1;")
+---
+- metadata:
+  - name: A
+    type: integer
+  rows: []
+...
+box.execute("SELECT a FROM t1 WHERE a > 1.1;")
+---
+- metadata:
+  - name: A
+    type: integer
+  rows: []
+...
+box.execute("SELECT a FROM t1 WHERE a < 1.1;")
+---
+- metadata:
+  - name: A
+    type: integer
+  rows:
+  - [1]
+...
+box.space.T1:drop()
+---
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 3c7bd5ea3..afa3677f0 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -228,3 +228,12 @@ box.execute("SELECT * FROM tboolean WHERE s1 = 1;")
 box.execute("SELECT * FROM tboolean WHERE s1 = 1.123;")
 
 box.space.TBOOLEAN:drop()
+
+box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
+box.execute("INSERT INTO t1 VALUES (1, 1);")
+box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
+box.execute("SELECT a FROM t1 WHERE a = 1.1;")
+box.execute("SELECT a FROM t1 WHERE a > 1.1;")
+box.execute("SELECT a FROM t1 WHERE a < 1.1;")
+
+box.space.T1:drop()
-- 
2.15.1

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/05/24 20:42]:
> That happened due to the fact that type casting mechanism (OP_ApplyType)
> doesn't affect FP value when it is converted to integer. Hence, FP value
> was passed to the iterator over integer field which resulted in error.
> Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
> with this problem for each equality comparison involving integer field
> we emit OP_MustBeInt, which checks whether value to be compared is
> integer or not. If the latter, we assume that result of comparison is
> always false and continue processing query. 

Are you sure other vendords would fail to return any results for
WHERE foo = 1.0?



-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Vladislav Shpilevoy]

On 25/05/2019 08:51, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/05/24 20:42]:
>> That happened due to the fact that type casting mechanism (OP_ApplyType)
>> doesn't affect FP value when it is converted to integer. Hence, FP value
>> was passed to the iterator over integer field which resulted in error.
>> Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
>> with this problem for each equality comparison involving integer field
>> we emit OP_MustBeInt, which checks whether value to be compared is
>> integer or not. If the latter, we assume that result of comparison is
>> always false and continue processing query. 
> 
> Are you sure other vendords would fail to return any results for
> WHERE foo = 1.0?

I do not understand, what you are talking about. It works.

tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
---
- row_count: 1
...

tarantool> box.execute("INSERT INTO t1 VALUES (1, 1);")
---
- row_count: 1
...

tarantool> box.execute("SELECT a FROM t1 WHERE a = 1.0;")
---
- metadata:
  - name: A
    type: integer
  rows:
  - [1]
...

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/05/28 09:22]:
> On 25/05/2019 08:51, Konstantin Osipov wrote:
> > * Nikita Pettik <korablev@tarantool.org> [19/05/24 20:42]:
> >> That happened due to the fact that type casting mechanism (OP_ApplyType)
> >> doesn't affect FP value when it is converted to integer. Hence, FP value
> >> was passed to the iterator over integer field which resulted in error.
> >> Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
> >> with this problem for each equality comparison involving integer field
> >> we emit OP_MustBeInt, which checks whether value to be compared is
> >> integer or not. If the latter, we assume that result of comparison is
> >> always false and continue processing query. 
> > 
> > Are you sure other vendords would fail to return any results for
> > WHERE foo = 1.0?
> 
> I do not understand, what you are talking about. It works.

Is this with the patch applied? 

> 
> tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
> tarantool> box.execute("INSERT INTO t1 VALUES (1, 1);")
> tarantool> box.execute("SELECT a FROM t1 WHERE a = 1.0;")

I don't understand how it works then.
a = 1.0 works but a = 1.1 doesn't?


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Vladislav Shpilevoy]

On 28/05/2019 10:19, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/05/28 09:22]:
>> On 25/05/2019 08:51, Konstantin Osipov wrote:
>>> * Nikita Pettik <korablev@tarantool.org> [19/05/24 20:42]:
>>>> That happened due to the fact that type casting mechanism (OP_ApplyType)
>>>> doesn't affect FP value when it is converted to integer. Hence, FP value
>>>> was passed to the iterator over integer field which resulted in error.
>>>> Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
>>>> with this problem for each equality comparison involving integer field
>>>> we emit OP_MustBeInt, which checks whether value to be compared is
>>>> integer or not. If the latter, we assume that result of comparison is
>>>> always false and continue processing query. 
>>>
>>> Are you sure other vendords would fail to return any results for
>>> WHERE foo = 1.0?
>>
>> I do not understand, what you are talking about. It works.
> 
> Is this with the patch applied? 

I do not know, and it does not matter, IMO. With the patch
it works.

> 
>>
>> tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
>> tarantool> box.execute("INSERT INTO t1 VALUES (1, 1);")
>> tarantool> box.execute("SELECT a FROM t1 WHERE a = 1.0;")
> 
> I don't understand how it works then.
> a = 1.0 works but a = 1.1 doesn't?

Yes, it is a school math. 1 != 1.1, but 1 == 1.0

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/05/28 14:33]:
> Yes, it is a school math. 1 != 1.1, but 1 == 1.0
> 

It's a different data type. So I don't understand how the type
system can "see" the actual value and act as an oracle. If there
is a type error, it should not depend on the value, should it?

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[n.pettik]

> On 28 May 2019, at 10:19, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/05/28 09:22]:
>> On 25/05/2019 08:51, Konstantin Osipov wrote:
>>> * Nikita Pettik <korablev@tarantool.org> [19/05/24 20:42]:
>>>> That happened due to the fact that type casting mechanism (OP_ApplyType)
>>>> doesn't affect FP value when it is converted to integer. Hence, FP value
>>>> was passed to the iterator over integer field which resulted in error.
>>>> Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
>>>> with this problem for each equality comparison involving integer field
>>>> we emit OP_MustBeInt, which checks whether value to be compared is
>>>> integer or not. If the latter, we assume that result of comparison is
>>>> always false and continue processing query. 
>>> 
>>> Are you sure other vendords would fail to return any results for
>>> WHERE foo = 1.0?
>> 
>> I do not understand, what you are talking about. It works.
> 
> Is this with the patch applied?

This behaviour is observed even without applied patch:

CREATE TABLE t(id INT PRIMARY KEY, a INT);
INSERT INTO t VALUES(1, 1);
SELECT * FROM t WHERE id = 1.1;
  rows: []
SELECT * FROM t WHERE id = 1.0;
  rows:
  - [1, 1]

SELECT * FROM t WHERE a = 1.1;
  rows: []
SELECT * FROM t WHERE a = 1.0;
  rows:
  - [1, 1]

But if we create index on field ‘a’, iterator will be
used instead of full scan. However, usage of
index search will result in error:

CREATE INDEX i1 on t(a);
SELECT * FROM t WHERE a = 1.1;
2019-05-28 15:51:40.974 [38764] main/102/interactive key_def.h:511 E> ER_KEY_PART_TYPE: Supplied key type of part 0 does not match index part type: expected integer
---
- error: 'Failed to execute SQL statement: Supplied key type of part 0 does not match
    index part type: expected integer'
…

Current patch fixes this misbehaviour and being applied
result of query above is the same as using full scan.

>> 
>> tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
>> tarantool> box.execute("INSERT INTO t1 VALUES (1, 1);")
>> tarantool> box.execute("SELECT a FROM t1 WHERE a = 1.0;")
> 
> I don't understand how it works then.
> a = 1.0 works but a = 1.1 doesn’t?

Ok, consider searching condition ‘a == FP’, where a is
integer field and FP is floating point value. Since both
sides of comparison operator have different numeric
types, one of them is promoted to another according to
precedence list (I use DB2 terminology:
https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tpc/db2z_promotionofdatatypes.html
I suppose it is not significantly different from ANSI but
way much easier to understand). In our case, integer is
promoted to float type. Hence, searching condition
is transformed to this one: ‘implicit_cast(a to float) == FP’.
In our particular case: ‘1.0 == 1.1’ and ‘1.0 == 1.0’.
First one is obviously false, second one it true.

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/05/28 16:01]:

> Ok, consider searching condition ‘a == FP’, where a is
> integer field and FP is floating point value. Since both
> sides of comparison operator have different numeric
> types, one of them is promoted to another according to
> precedence list (I use DB2 terminology:
> https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tpc/db2z_promotionofdatatypes.html
> I suppose it is not significantly different from ANSI but
> way much easier to understand). In our case, integer is
> promoted to float type. Hence, searching condition
> is transformed to this one: ‘implicit_cast(a to float) == FP’.
> In our particular case: ‘1.0 == 1.1’ and ‘1.0 == 1.0’.
> First one is obviously false, second one it true.

Oh, I missed that implicit casts between integer
and float are allowed and promotion rules apply.

I thought it's strings and numbers only, not the different numbers
between each other. It is my mistake.

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Vladislav Shpilevoy]

Thanks for the patch! See 7 comments below.

1. Double whitespace in the commit title before 'FP'.

2. The patch does not work with unsigned numbers.

		tarantool> format = {}
		---
		...

		tarantool> format[1] = {name = 'ID', type = 'unsigned'}
		---
		...

		tarantool> format[2] = {name = 'A', type = 'unsigned'}
		---
		...

		tarantool> s = box.schema.create_space('T1', {format = format})
		---
		...

		tarantool> pk = s:create_index('pk')
		---
		...

		tarantool> sk = s:create_index('sk', {parts = {'A'}})
		---
		...

		tarantool> box.execute("INSERT INTO t1 VALUES (1, 1);")
		---
		- row_count: 1
		...

		tarantool> box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
		2019-05-28 00:34:19.922 [1452] main/102/interactive key_def.h:488 E> ER_KEY_PART_TYPE: Supplied key type of part 0 does not match index part type: expected unsigned
		---
		- error: 'Failed to execute SQL statement: Supplied key type of part 0 does not match
		    index part type: expected unsigned'
		...

On 24/05/2019 20:39, Nikita Pettik wrote:
> Before this patch it was impossible to compare indexed field of integer
> type and floating point value. For instance:
> 
> CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);
> INSERT INTO t1 VALUES (1, 1);
> SELECT * FROM t1 WHERE a = 1.5;
> ---
> - error: 'Failed to execute SQL statement: Supplied key type of part 0 does not match
>     index part type: expected integer'
> ...
> 
> That happened due to the fact that type casting mechanism (OP_ApplyType)
> doesn't affect FP value when it is converted to integer. Hence, FP value
> was passed to the iterator over integer field which resulted in error.
> Meanwhile, comparison of integer and FP values is legal in SQL.  To cope
> with this problem for each equality comparison involving integer field
> we emit OP_MustBeInt, which checks whether value to be compared is
> integer or not. If the latter, we assume that result of comparison is
> always false and continue processing query.  For inequality constraints
> we pass auxiliary flag to OP_Seek** opcodes to notify it that one of key
> fields must be truncated to integer (in case of FP value) alongside with
> changing iterator's type: a > 1.5 -> a >= 2.
> 
> Closes #4187
> ---
>  src/box/sql/vdbe.c      | 23 +++++++++------
>  src/box/sql/wherecode.c | 76 +++++++++++++++++++++++++++++++++++--------------
>  test/sql/types.result   | 40 ++++++++++++++++++++++++++
>  test/sql/types.test.lua |  9 ++++++
>  4 files changed, 118 insertions(+), 30 deletions(-)
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3bd82234e..3ead6b40f 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3496,15 +3495,22 @@ case OP_SeekGT: {       /* jump, in3 */
>  	pC->seekOp = pOp->opcode;
>  #endif
>  	iKey = 0;
> -	reg_ipk = pOp->p5;
> -
> -	if (reg_ipk > 0) {
> +	/*
> +	 * In case floating value is intended to be passed to
> +	 * iterator over integer field, we must truncate it to
> +	 * integer value and change type of iterator:
> +	 * a > 1.5 -> a >= 2
> +	 */
> +	int int_field = pOp->p5;

3. Comments on P5 are now outdated.

>  
> +	if (int_field > 0) {
>  		/* The input value in P3 might be of any type: integer, real, string,
>  		 * blob, or NULL.  But it needs to be an integer before we can do
>  		 * the seek, so convert it.
>  		 */
> -		pIn3 = &aMem[reg_ipk];
> +		pIn3 = &aMem[int_field];
> +		if ((pIn3->flags & MEM_Null) != 0)
> +			goto skip_truncate;
>  		if ((pIn3->flags & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
>  			mem_apply_numeric_type(pIn3);
>  		}
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 977c0fced..e779729c7 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>  			start_constraints = 1;
>  		}
>  		struct index_def *idx_pk = space->index[0]->def;
> -		int fieldno = idx_pk->key_def->parts[0].fieldno;
> -
>  		uint32_t pk_part_count = idx_pk->key_def->part_count;
> -		if (pk_part_count == 1 &&
> -		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
> -			/* Right now INTEGER PRIMARY KEY is the only option to
> -			 * get Tarantool's INTEGER column type. Need special handling
> -			 * here: try to loosely convert FLOAT to INT. If RHS type
> -			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
> -			 */
> -			int limit = pRangeStart == NULL ? nEq : nEq + 1;
> -			for (int i = 0; i < limit; i++) {
> -				if (idx_def->key_def->parts[i].fieldno ==
> -				    idx_pk->key_def->parts[0].fieldno) {
> -					/* Here: we know for sure that table has INTEGER
> -					   PRIMARY KEY, single column, and Index we're
> -					   trying to use for scan contains this column. */
> -					if (i < nEq)
> -						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
> -					else
> -						force_integer_reg = regBase + i;
> -					break;
> -				}
> +		/*
> +		 * 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:
> +		 * 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.

4. I did not get the 'less comparator == compare with NULL'
idea. Firstly you describe how to compare 'a > ?', this is
fully understandable. Then you, as I see, go to the 'a < ?'
case, but somewhy say, that it is the same as 'NULL < ?'.
Do not understand.

> +		 * 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.

5. Why can't OP_ApplyType truncate the numbers? Because it does
not know, to which side round the values? Then probably we can
drop OP_ApplyType invocations?

> +		 * Also, note that terms are separated by OR
> +		 * predicates, so we consider term as sequence
> +		 * of AND'ed predicates.
> +		 */
> +		int seek_addr = 0;
> +		for (int i = 0; i < nEq; i++) {
> +			enum field_type type = idx_def->key_def->parts[i].type;
> +			if (type == FIELD_TYPE_INTEGER) {
> +				/*
> +				 * OP_MustBeInt consider NULLs as
> +				 * non-integer values, so firstly
> +				 * check whether value is NULL or not.
> +				 */
> +				seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
> +							  regBase);

6. I do not understand, how does it work. You rewrite
seek_addr in the cycle. It means, that if there are 2 integers,
the first OP_IsNull address is rewritten by the second, and
below, in sqlVdbeJumpHere(v, seek_addr), you will update P2
for the last comparison only. The first OP_IsNull.P2 will stay 0.

> +				sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
> +					      addrNxt);
>  			}
>  		}
> +		/* Inequality constraint comes always at the end of list. */
> +		if (pRangeStart != NULL &&
> +		    idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
> +			force_integer_reg = regBase + nEq;
>  		emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
>  				start_types);
>  		if (pLoop->nSkip > 0 && nConstraint == pLoop->nSkip) {
> @@ -1122,6 +1152,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>  			op = aStartOp[(start_constraints << 2) +
>  				      (startEq << 1) + bRev];
>  			assert(op != 0);
> +			if (seek_addr != 0)
> +				sqlVdbeJumpHere(v, seek_addr);
>  			sqlVdbeAddOp4Int(v, op, iIdxCur, addrNxt, regBase,
>  					     nConstraint);
>  			/* If this is Seek* opcode, and IPK is detected in the
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 758018eb5..274d73dd2 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -927,3 +927,43 @@ box.execute("SELECT * FROM tboolean WHERE s1 = 1.123;")
>  box.space.TBOOLEAN:drop()
>  ---
>  ...
> +box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
> +---
> +- row_count: 1
> +...
> +box.execute("INSERT INTO t1 VALUES (1, 1);")
> +---
> +- row_count: 1
> +...
> +box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
> +---
> +- metadata:
> +  - name: A
> +    type: integer
> +  rows: []
> +...
> +box.execute("SELECT a FROM t1 WHERE a = 1.1;")
> +---
> +- metadata:
> +  - name: A
> +    type: integer
> +  rows: []
> +...
> +box.execute("SELECT a FROM t1 WHERE a > 1.1;")
> +---
> +- metadata:
> +  - name: A
> +    type: integer
> +  rows: []
> +...
> +box.execute("SELECT a FROM t1 WHERE a < 1.1;")
> +---
> +- metadata:
> +  - name: A
> +    type: integer
> +  rows:
> +  - [1]

7. Please, add the Kostja's example about '= 1.0'.

> +...
> +box.space.T1:drop()
> +---
> +...

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[n.pettik]

> On 28 May 2019, at 00:49, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the patch! See 7 comments below.
> 
> 1. Double whitespace in the commit title before 'FP’.

Fixed.

> 
> 2. The patch does not work with unsigned numbers.
> 
> 		tarantool> format = {}
> 		tarantool> format[1] = {name = 'ID', type = 'unsigned'}
> 		tarantool> format[2] = {name = 'A', type = 'unsigned'}
> 		tarantool> s = box.schema.create_space('T1', {format = format})
> 		tarantool> pk = s:create_index('pk')
> 		tarantool> sk = s:create_index('sk', {parts = {'A'}})
> 		tarantool> box.execute("INSERT INTO t1 VALUES (1, 1);")
> 		tarantool> box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
> 		2019-05-28 00:34:19.922 [1452] main/102/interactive key_def.h:488 E> ER_KEY_PART_TYPE: Supplied key type of part 0 does not match index part type: expected unsigned
> 		---
> 		- error: 'Failed to execute SQL statement: Supplied key type of part 0 does not match
> 		    index part type: expected unsigned’

Fixed:

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 3025e5e6b..107b9802d 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1115,7 +1115,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
                int seek_addr = 0;
                for (int i = 0; i < nEq; i++) {
                        enum field_type type = idx_def->key_def->parts[i].type;
-                       if (type == FIELD_TYPE_INTEGER) {
+                       if (type == FIELD_TYPE_INTEGER ||
+                           type == FIELD_TYPE_UNSIGNED) {
                                /*
                                 * OP_MustBeInt consider NULLs as
                                 * non-integer values, so firstly
@@ -1128,8 +1129,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
                        }
                }
                /* Inequality constraint comes always at the end of list. */
-               if (pRangeStart != NULL &&
-                   idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
+               enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
+               if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
+                                           ineq_type == FIELD_TYPE_UNSIGNED))
                        force_integer_reg = regBase + nEq;
                emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
                                start_types);

>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 3bd82234e..3ead6b40f 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -3496,15 +3495,22 @@ case OP_SeekGT: {       /* jump, in3 */
>> 	pC->seekOp = pOp->opcode;
>> #endif
>> 	iKey = 0;
>> -	reg_ipk = pOp->p5;
>> -
>> -	if (reg_ipk > 0) {
>> +	/*
>> +	 * In case floating value is intended to be passed to
>> +	 * iterator over integer field, we must truncate it to
>> +	 * integer value and change type of iterator:
>> +	 * a > 1.5 -> a >= 2
>> +	 */
>> +	int int_field = pOp->p5;
> 
> 3. Comments on P5 are now outdated.

Fixed:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3ead6b40f..906baa6b1 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3395,8 +3395,10 @@ case OP_ColumnsUsed: {
  * 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 IPK in input vector. Force
- * corresponding value to be INTEGER.
+ * 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
  */
@@ -3416,10 +3418,10 @@ case OP_ColumnsUsed: {
  * 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 IPK in input vector. Force
- * corresponding value to be INTEGER.
+ * If P5 is not zero, than it is offset of integer fields in input
+ * vector. Force corresponding value to be INTEGER.
  *
- * See also: Found, NotFound, SeekLt, SeekGe, SeekLe
+ * P5 has the same meaning as for SeekGE.
  */
 /* Opcode: SeekLT P1 P2 P3 P4 P5
  * Synopsis: key=r[P3@P4]
@@ -3437,8 +3439,7 @@ case OP_ColumnsUsed: {
  * from the end toward the beginning.  In other words, the cursor is
  * configured to use Prev, not Next.
  *
- * If P5 is not zero, than it is offset of IPK in input vector. Force
- * corresponding value to be INTEGER.
+ * P5 has the same meaning as for SeekGE.
  *
  * See also: Found, NotFound, SeekGt, SeekGe, SeekLe
  */
@@ -3465,6 +3466,8 @@ case OP_ColumnsUsed: {
  * 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_SeekLT:         /* jump, in3 */

>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index 977c0fced..e779729c7 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>> 			start_constraints = 1;
>> 		}
>> 		struct index_def *idx_pk = space->index[0]->def;
>> -		int fieldno = idx_pk->key_def->parts[0].fieldno;
>> -
>> 		uint32_t pk_part_count = idx_pk->key_def->part_count;
>> -		if (pk_part_count == 1 &&
>> -		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
>> -			/* Right now INTEGER PRIMARY KEY is the only option to
>> -			 * get Tarantool's INTEGER column type. Need special handling
>> -			 * here: try to loosely convert FLOAT to INT. If RHS type
>> -			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
>> -			 */
>> -			int limit = pRangeStart == NULL ? nEq : nEq + 1;
>> -			for (int i = 0; i < limit; i++) {
>> -				if (idx_def->key_def->parts[i].fieldno ==
>> -				    idx_pk->key_def->parts[0].fieldno) {
>> -					/* Here: we know for sure that table has INTEGER
>> -					   PRIMARY KEY, single column, and Index we're
>> -					   trying to use for scan contains this column. */
>> -					if (i < nEq)
>> -						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
>> -					else
>> -						force_integer_reg = regBase + i;
>> -					break;
>> -				}
>> +		/*
>> +		 * 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:
>> +		 * 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.
> 
> 4. I did not get the 'less comparator == compare with NULL'
> idea. Firstly you describe how to compare 'a > ?', this is
> fully understandable. Then you, as I see, go to the 'a < ?'
> case, but somewhy say, that it is the same as 'NULL < ?'.
> Do not understand.

I say that in this case key which is passed to iterator is NULL
and type of iterator is GE. Consequently, pRangeStart is NULL.
I’ve slightly updated this part of comment:

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 3025e5e6b..01ee9ce90 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1089,10 +1089,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
                 * 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:
-                * 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.
+                * 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.
> 
> 5. Why can't OP_ApplyType truncate the numbers? Because it does
> not know, to which side round the values?

It doesn’t (and can't) change type of iterator alongside with truncation.

> Then probably we can drop OP_ApplyType invocations?

We can’t remove whole invocation to OP_ApplyType, but
we can really skip cast to int:

                                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);


Idk whether we need such optimisation or not

>> +		 * Also, note that terms are separated by OR
>> +		 * predicates, so we consider term as sequence
>> +		 * of AND'ed predicates.
>> +		 */
>> +		int seek_addr = 0;
>> +		for (int i = 0; i < nEq; i++) {
>> +			enum field_type type = idx_def->key_def->parts[i].type;
>> +			if (type == FIELD_TYPE_INTEGER) {
>> +				/*
>> +				 * OP_MustBeInt consider NULLs as
>> +				 * non-integer values, so firstly
>> +				 * check whether value is NULL or not.
>> +				 */
>> +				seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
>> +							  regBase);
> 
> 6. I do not understand, how does it work. You rewrite
> seek_addr in the cycle. It means, that if there are 2 integers,
> the first OP_IsNull address is rewritten by the second, and
> below, in sqlVdbeJumpHere(v, seek_addr), you will update P2
> for the last comparison only. The first OP_IsNull.P2 will stay 0.

You are right. Fixed:

@@ -1112,24 +1112,33 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
                 * predicates, so we consider term as sequence
                 * of AND'ed predicates.
                 */
-               int seek_addr = 0;
+               size_t addrs_sz = sizeof(int) * nEq;
+               int *seek_addrs = region_alloc(&pParse->region, addrs_sz);
+               if (seek_addrs == NULL) {
+                       diag_set(OutOfMemory, addrs_sz, "region", "seek_addrs");
+                       pParse->is_aborted = true;
+                       return 0;
+               }
+               memset(seek_addrs, 0, addrs_sz);
...
                                /*
                                 * OP_MustBeInt consider NULLs as
                                 * non-integer values, so firstly
                                 * check whether value is NULL or not.
                                 */
-                               seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
+                               seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
                                                          regBase);
                                sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
                                              addrNxt);
                        }
                }
                /* Inequality constraint comes always at the end of list. */

@@ -1142,8 +1151,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,     /* Complete information about the W
                        op = aStartOp[(start_constraints << 2) +
                                      (startEq << 1) + bRev];
                        assert(op != 0);
-                       if (seek_addr != 0)
-                               sqlVdbeJumpHere(v, seek_addr);
+                       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);
And added test case:

diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index afa3677f0..170a1ad04 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -237,3 +237,12 @@ box.execute("SELECT a FROM t1 WHERE a > 1.1;")
 box.execute("SELECT a FROM t1 WHERE a < 1.1;")
 
 box.space.T1:drop()
+
+box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT, b INT);")
+box.execute("CREATE INDEX i1 ON t1(a, b);")
+box.execute("INSERT INTO t1 VALUES (1, 1, 1);")
+box.execute("SELECT a FROM t1 WHERE a = 1.0 AND b > 0.5;")
+box.execute("SELECT a FROM t1 WHERE a = 1.5 AND b IS NULL;")
+box.execute("SELECT a FROM t1 WHERE a IS NULL AND b IS NULL;")
+
+box.space.T1:drop()

> 
>> +				sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
>> +					      addrNxt);
>> 			}
>> 		}
>> +		/* Inequality constraint comes always at the end of list. */
>> +		if (pRangeStart != NULL &&
>> +		    idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
>> +			force_integer_reg = regBase + nEq;
>> 		emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
>> 				start_types);
>> 		if (pLoop->nSkip > 0 && nConstraint == pLoop->nSkip) {
>> 
>> diff --git a/test/sql/types.result b/test/sql/types.result
>> index 758018eb5..274d73dd2 100644
>> --- a/test/sql/types.result
>> +++ b/test/sql/types.result
>> @@ -927,3 +927,43 @@ box.execute("SELECT * FROM tboolean WHERE s1 = 1.123;")
>> +...
>> +box.execute("SELECT a FROM t1 WHERE a < 1.1;")
>> +---
>> +- metadata:
>> +  - name: A
>> +    type: integer
>> +  rows:
>> +  - [1]
> 
> 7. Please, add the Kostja's example about '= 1.0’.

diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index afa3677f0..16448436d 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -233,7 +233,17 @@ box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
 box.execute("INSERT INTO t1 VALUES (1, 1);")
 box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
 box.execute("SELECT a FROM t1 WHERE a = 1.1;")
+box.execute("SELECT a FROM t1 WHERE a = 1.0;")
 box.execute("SELECT a FROM t1 WHERE a > 1.1;")
 box.execute("SELECT a FROM t1 WHERE a < 1.1;")

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! See 3 comments below.

> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 3025e5e6b..107b9802d 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1115,7 +1115,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
>                 int seek_addr = 0;
>                 for (int i = 0; i < nEq; i++) {
>                         enum field_type type = idx_def->key_def->parts[i].type;
> -                       if (type == FIELD_TYPE_INTEGER) {
> +                       if (type == FIELD_TYPE_INTEGER ||
> +                           type == FIELD_TYPE_UNSIGNED) {
>                                 /*
>                                  * OP_MustBeInt consider NULLs as
>                                  * non-integer values, so firstly
> @@ -1128,8 +1129,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
>                         }
>                 }
>                 /* Inequality constraint comes always at the end of list. */
> -               if (pRangeStart != NULL &&
> -                   idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
> +               enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
> +               if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
> +                                           ineq_type == FIELD_TYPE_UNSIGNED))

1. Please, add a test for unsigned. I thought that it is quite obvious -
everything needs a test, especially if the bug was found during a review.

>                         force_integer_reg = regBase + nEq;
>                 emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull,
>                                 start_types);
> 
>>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>>> index 977c0fced..e779729c7 100644
>>> --- a/src/box/sql/wherecode.c
>>> +++ b/src/box/sql/wherecode.c
>>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>>> 			start_constraints = 1;
>>> 		}
>>> 		struct index_def *idx_pk = space->index[0]->def;
>>> -		int fieldno = idx_pk->key_def->parts[0].fieldno;
>>> -
>>> 		uint32_t pk_part_count = idx_pk->key_def->part_count;
>>> -		if (pk_part_count == 1 &&
>>> -		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
>>> -			/* Right now INTEGER PRIMARY KEY is the only option to
>>> -			 * get Tarantool's INTEGER column type. Need special handling
>>> -			 * here: try to loosely convert FLOAT to INT. If RHS type
>>> -			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
>>> -			 */
>>> -			int limit = pRangeStart == NULL ? nEq : nEq + 1;
>>> -			for (int i = 0; i < limit; i++) {
>>> -				if (idx_def->key_def->parts[i].fieldno ==
>>> -				    idx_pk->key_def->parts[0].fieldno) {
>>> -					/* Here: we know for sure that table has INTEGER
>>> -					   PRIMARY KEY, single column, and Index we're
>>> -					   trying to use for scan contains this column. */
>>> -					if (i < nEq)
>>> -						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
>>> -					else
>>> -						force_integer_reg = regBase + i;
>>> -					break;
>>> -				}
>>> +		/*
>>> +		 * 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:
>>> +		 * 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.
>>
>> 4. I did not get the 'less comparator == compare with NULL'
>> idea. Firstly you describe how to compare 'a > ?', this is
>> fully understandable. Then you, as I see, go to the 'a < ?'
>> case, but somewhy say, that it is the same as 'NULL < ?'.
>> Do not understand.
> 
> I say that in this case key which is passed to iterator is NULL
> and type of iterator is GE.

2. Why? I still do not understand. What if I wrote 'a < 100'? you
do not replace 100 with NULL. But the comment says, that for any
'less' operator a key is NULL: "when it comes for less comparison
key is NULL".

> Consequently, pRangeStart is NULL.
> I’ve slightly updated this part of comment:
> 
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 3025e5e6b..01ee9ce90 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1089,10 +1089,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
>                  * 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:
> -                * 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.
> +                * 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.
> 
> 
> @@ -1112,24 +1112,33 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
>                                 /*
>                                  * OP_MustBeInt consider NULLs as
>                                  * non-integer values, so firstly
>                                  * check whether value is NULL or not.
>                                  */
> -                               seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
> +                               seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
>                                                           regBase);

3. Now the indentation is broken.

>                                 sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
>                                               addrNxt);
>                         }
>                 }
>                 /* Inequality constraint comes always at the end of list. */
> 

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[n.pettik]

>> @@ -1128,8 +1129,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
>>                        }
>>                }
>>                /* Inequality constraint comes always at the end of list. */
>> -               if (pRangeStart != NULL &&
>> -                   idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER)
>> +               enum field_type ineq_type = idx_def->key_def->parts[nEq].type;
>> +               if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER ||
>> +                                           ineq_type == FIELD_TYPE_UNSIGNED))
> 
> 1. Please, add a test for unsigned. I thought that it is quite obvious -
> everything needs a test, especially if the bug was found during a review.

Surely, forgot about that:

diff --git a/test/sql/types.result b/test/sql/types.result
index bba39e320..383f93c1a 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1012,3 +1012,35 @@ box.execute("SELECT a FROM t1 WHERE a IS NULL AND b IS NULL;")
 box.space.T1:drop()
 ---
 ...
+format = {}
+---
+...
+format[1] = { name = 'ID', type = 'unsigned' }
+---
+...
+format[2] = { name = 'A', type = 'unsigned' }
+---
+...
+s = box.schema.create_space('T1', { format = format })
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', { parts = { 'A' } })
+---
+...
+s:insert({ 1, 1 })
+---
+- [1, 1]
+...
+box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
+---
+- metadata:
+  - name: A
+    type: unsigned
+  rows: []
+...
+s:drop()
+---
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 16448436d..0218c5498 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -247,3 +247,14 @@ box.execute("SELECT a FROM t1 WHERE a = 1.5 AND b IS NULL;")
 box.execute("SELECT a FROM t1 WHERE a IS NULL AND b IS NULL;")
 
 box.space.T1:drop()
+
+format = {}
+format[1] = { name = 'ID', type = 'unsigned' }
+format[2] = { name = 'A', type = 'unsigned' }
+s = box.schema.create_space('T1', { format = format })
+_ = s:create_index('pk')
+_ = s:create_index('sk', { parts = { 'A' } })
+s:insert({ 1, 1 })
+box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
+
+s:drop()

>>>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>>>> index 977c0fced..e779729c7 100644
>>>> --- a/src/box/sql/wherecode.c
>>>> +++ b/src/box/sql/wherecode.c
>>>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>>>> 			start_constraints = 1;
>>>> 		}
>>>> 		struct index_def *idx_pk = space->index[0]->def;
>>>> -		int fieldno = idx_pk->key_def->parts[0].fieldno;
>>>> -
>>>> 		uint32_t pk_part_count = idx_pk->key_def->part_count;
>>>> -		if (pk_part_count == 1 &&
>>>> -		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
>>>> -			/* Right now INTEGER PRIMARY KEY is the only option to
>>>> -			 * get Tarantool's INTEGER column type. Need special handling
>>>> -			 * here: try to loosely convert FLOAT to INT. If RHS type
>>>> -			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
>>>> -			 */
>>>> -			int limit = pRangeStart == NULL ? nEq : nEq + 1;
>>>> -			for (int i = 0; i < limit; i++) {
>>>> -				if (idx_def->key_def->parts[i].fieldno ==
>>>> -				    idx_pk->key_def->parts[0].fieldno) {
>>>> -					/* Here: we know for sure that table has INTEGER
>>>> -					   PRIMARY KEY, single column, and Index we're
>>>> -					   trying to use for scan contains this column. */
>>>> -					if (i < nEq)
>>>> -						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
>>>> -					else
>>>> -						force_integer_reg = regBase + i;
>>>> -					break;
>>>> -				}
>>>> +		/*
>>>> +		 * 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:
>>>> +		 * 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.
>>> 
>>> 4. I did not get the 'less comparator == compare with NULL'
>>> idea. Firstly you describe how to compare 'a > ?', this is
>>> fully understandable. Then you, as I see, go to the 'a < ?'
>>> case, but somewhy say, that it is the same as 'NULL < ?'.
>>> Do not understand.
>> 
>> I say that in this case key which is passed to iterator is NULL
>> and type of iterator is GE.
> 
> 2. Why? I still do not understand. What if I wrote 'a < 100'? you
> do not replace 100 with NULL. But the comment says, that for any
> 'less' operator a key is NULL: "when it comes for less comparison
> key is NULL”.

Consider example:

box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
box.execute("INSERT INTO t1 VALUES (1, 1), (2, 2), (3, -1), (4, NULL), (5, NULL);”)
box.execute("SELECT a FROM t1 WHERE a < 100;”)

Bytecode for this query is:

 102>    0 Init             0    1    0               00 Start at 1
 102>    1 IteratorOpen     1    1    0 space<name=T1> 00 index id = 1, space ptr = space<name=T1> or reg[0]; unique_unnamed_T1_2
 102>    2 Explain          0    0    0 SEARCH TABLE T1 USING COVERING INDEX unique_unnamed_T1_2 (A<?) 00 
 102>    3 Null             0    1    0               00 r[1]=NULL
 102>    4 SeekGT           1   10    1 1             00 key=r[1]
 102>    5 Integer        100    1    0               00 r[1]=100
 102>    6 IdxGE            1   10    1 1             00 key=r[1]
 102>    7 Column           1    1    2               00 r[2]=T1.A
 102>    8 ResultRow        2    1    0               00 output=r[2]
 102>    9 Next             1    6    0               00 
 102>   10 Halt             0    0    0               00 

As you can see, SeekGT accepts NULL as a key for iterator.
Then we iterate until search condition is true. So, basically
100 is replaced with NULL and not used as a key of iterator.
I wanted to underline this fact in my comment.

>> Consequently, pRangeStart is NULL.
>> I’ve slightly updated this part of comment:
>> 
>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index 3025e5e6b..01ee9ce90 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -1089,10 +1089,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
>>                 * 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:
>> -                * 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.
>> +                * 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.
>> 
>> 
>> @@ -1112,24 +1112,33 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,    /* Complete information about the W
>>                                /*
>>                                 * OP_MustBeInt consider NULLs as
>>                                 * non-integer values, so firstly
>>                                 * check whether value is NULL or not.
>>                                 */
>> -                               seek_addr = sqlVdbeAddOp1(v, OP_IsNull,
>> +                               seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
>>                                                          regBase);
> 
> 3. Now the indentation is broken.

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index fde2efee0..4bd14bd5f 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1130,7 +1130,7 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
                                 * check whether value is NULL or not.
                                 */
                                seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull,
-                                                         regBase);
+                                                             regBase);
                                sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i,
                                              addrNxt);
                                start_types[i] = FIELD_TYPE_SCALAR;

[tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator

[Vladislav Shpilevoy]

Thanks for the fixes!

>>>>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>>>>> index 977c0fced..e779729c7 100644
>>>>> --- a/src/box/sql/wherecode.c
>>>>> +++ b/src/box/sql/wherecode.c
>>>>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>>>>> 			start_constraints = 1;
>>>>> 		}
>>>>> 		struct index_def *idx_pk = space->index[0]->def;
>>>>> -		int fieldno = idx_pk->key_def->parts[0].fieldno;
>>>>> -
>>>>> 		uint32_t pk_part_count = idx_pk->key_def->part_count;
>>>>> -		if (pk_part_count == 1 &&
>>>>> -		    space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) {
>>>>> -			/* Right now INTEGER PRIMARY KEY is the only option to
>>>>> -			 * get Tarantool's INTEGER column type. Need special handling
>>>>> -			 * here: try to loosely convert FLOAT to INT. If RHS type
>>>>> -			 * is not INT or FLOAT - skip this ites, i.e. goto addrNxt.
>>>>> -			 */
>>>>> -			int limit = pRangeStart == NULL ? nEq : nEq + 1;
>>>>> -			for (int i = 0; i < limit; i++) {
>>>>> -				if (idx_def->key_def->parts[i].fieldno ==
>>>>> -				    idx_pk->key_def->parts[0].fieldno) {
>>>>> -					/* Here: we know for sure that table has INTEGER
>>>>> -					   PRIMARY KEY, single column, and Index we're
>>>>> -					   trying to use for scan contains this column. */
>>>>> -					if (i < nEq)
>>>>> -						sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt);
>>>>> -					else
>>>>> -						force_integer_reg = regBase + i;
>>>>> -					break;
>>>>> -				}
>>>>> +		/*
>>>>> +		 * 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:
>>>>> +		 * 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.
>>>>
>>>> 4. I did not get the 'less comparator == compare with NULL'
>>>> idea. Firstly you describe how to compare 'a > ?', this is
>>>> fully understandable. Then you, as I see, go to the 'a < ?'
>>>> case, but somewhy say, that it is the same as 'NULL < ?'.
>>>> Do not understand.
>>>
>>> I say that in this case key which is passed to iterator is NULL
>>> and type of iterator is GE.
>>
>> 2. Why? I still do not understand. What if I wrote 'a < 100'? you
>> do not replace 100 with NULL. But the comment says, that for any
>> 'less' operator a key is NULL: "when it comes for less comparison
>> key is NULL”.
> 
> Consider example:
> 
> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);")
> box.execute("INSERT INTO t1 VALUES (1, 1), (2, 2), (3, -1), (4, NULL), (5, NULL);”)
> box.execute("SELECT a FROM t1 WHERE a < 100;”)
> 
> Bytecode for this query is:
> 
>  102>    0 Init             0    1    0               00 Start at 1
>  102>    1 IteratorOpen     1    1    0 space<name=T1> 00 index id = 1, space ptr = space<name=T1> or reg[0]; unique_unnamed_T1_2
>  102>    2 Explain          0    0    0 SEARCH TABLE T1 USING COVERING INDEX unique_unnamed_T1_2 (A<?) 00 
>  102>    3 Null             0    1    0               00 r[1]=NULL
>  102>    4 SeekGT           1   10    1 1             00 key=r[1]
>  102>    5 Integer        100    1    0               00 r[1]=100
>  102>    6 IdxGE            1   10    1 1             00 key=r[1]
>  102>    7 Column           1    1    2               00 r[2]=T1.A
>  102>    8 ResultRow        2    1    0               00 output=r[2]
>  102>    9 Next             1    6    0               00 
>  102>   10 Halt             0    0    0               00 
> 
> As you can see, SeekGT accepts NULL as a key for iterator.
> Then we iterate until search condition is true. So, basically
> 100 is replaced with NULL and not used as a key of iterator.
> I wanted to underline this fact in my comment.

Ah, thanks, now I got it.

Kirill, the patchset LGTM.

[tarantool-patches] Re: [PATCH 0/3] Fix passing key with different from iterator's type

[Kirill Yukhin]

Hello,

On 24 May 20:39, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/fix-bool-fp-to-int
> Issue: https://github.com/tarantool/tarantool/issues/4187
> 
> This patch-set fixes two bugs connected with passing to Tarantool
> iterators values of different from covered by iterator fields types.
> 
> Nikita Pettik (3):
>   sql: remove redundant check of space format from QP
>   sql: remove redundant type derivation from QP
>   sql: fix passing  FP values to integer iterator

I've checked your patch set into master.

--
Regards, Kirill Yukhin