[tarantool-patches] [PATCH 3/8] sql: remove numeric affinity

[Nikita Pettik <korablev@tarantool.org>]

Numeric affinity in SQLite means the same as real, except that it
forces integer values into floating point representation in case
it can be converted without loss (e.g. 2.0 -> 2).
Since in Tarantool core there is no difference between numeric and real
values (both are stored as values of type NUMBER), lets remove numeric
affinity and use instead real.

The only real pitfall is implicit conversion mentioned above.
What is more, vinyl engine complicates problem since it relies
on data encoding (i.e. whether it is encoded as MP_INT or MP_FLOAT).
For instance, if we encode 1.0 as MP_FLOAT during insertion, we won't
be able to use iterators from Lua, since they implicitly change type of
1.0 and pass it to the iterator as MP_INT.  Solution to this problem is
simple: lets always attempt at encoding floats as ints if conversion
takes place without loss. This is a straightforward approach, but to
implement it we need to care about reversed (decoding) situation.

OP_Column fetches from msgpack field with given number and stores it as
a native VDBE memory object. Type of that memory is based on type of
msgpack value. So, if space field is of type NUMBER and holds value 1,
type of VDBE memory will be INT (after decoding), not float 1.0.  As a
result, further calculations may be wrong: for instance, instead of
floating point division, we could get integer division.  To cope with
this problem, lets add auxiliary conversion to decoding routine which
uses space format of tuple to be decoded. It is worth mentioning that
ephemeral spaces don't feature space format, so we are going to rely on
type of key parts. Finally, internal VDBE merge sorter also operates on
entries encoded into msgpack. To fix this case, we check type of
ORDER BY/GROUP BY arguments: if they are of type float, we are emitting
additional opcode OP_AffinityReal to force float type after encoding.

Part of #3698
---
 src/box/field_def.h                  |  1 -
 src/box/lua/lua_sql.c                |  2 +-
 src/box/sql.c                        | 10 +++++++---
 src/box/sql/build.c                  |  1 -
 src/box/sql/expr.c                   | 20 +++++++++++---------
 src/box/sql/select.c                 | 10 +++++++---
 src/box/sql/sqliteInt.h              |  2 +-
 src/box/sql/vdbe.c                   | 26 ++++++++++++++++++--------
 src/box/sql/vdbeaux.c                | 19 ++++++++++++++++---
 src/box/sql/vdbemem.c                |  7 ++-----
 test/sql-tap/tkt-80e031a00f.test.lua | 12 ++++++------
 11 files changed, 69 insertions(+), 41 deletions(-)

diff --git a/src/box/field_def.h b/src/box/field_def.h
index bd3f9ba45..93e38ea55 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -77,7 +77,6 @@ enum affinity_type {
     AFFINITY_UNDEFINED = 0,
     AFFINITY_BLOB = 'A',
     AFFINITY_TEXT = 'B',
-    AFFINITY_NUMERIC = 'C',
     AFFINITY_INTEGER = 'D',
     AFFINITY_REAL = 'E',
 };
diff --git a/src/box/lua/lua_sql.c b/src/box/lua/lua_sql.c
index 5c77b8036..ab0b7f37c 100644
--- a/src/box/lua/lua_sql.c
+++ b/src/box/lua/lua_sql.c
@@ -158,7 +158,7 @@ lbox_sql_create_function(struct lua_State *L)
 	else if (strcmp(type_arg, "FLOAT") == 0)
 		type = AFFINITY_REAL;
 	else if (strcmp(type_arg, "NUM") == 0)
-		type = AFFINITY_NUMERIC;
+		type = AFFINITY_REAL;
 	else if (strcmp(type_arg, "BLOB") == 0)
 		type = AFFINITY_BLOB;
 	else
diff --git a/src/box/sql.c b/src/box/sql.c
index a06c50dca..a498cd8fe 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -376,14 +376,18 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	for (uint32_t i = 0; i < field_count; ++i) {
 		struct key_part_def *part = &ephemer_key_parts[i];
 		part->fieldno = i;
-		part->type = FIELD_TYPE_SCALAR;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
 		part->sort_order = SORT_ORDER_ASC;
-		if (def != NULL && i < def->part_count)
+		if (def != NULL && i < def->part_count) {
+			assert(def->parts[i].type < field_type_MAX);
+			part->type = def->parts[i].type != FIELD_TYPE_ANY ?
+				     def->parts[i].type : FIELD_TYPE_SCALAR;
 			part->coll_id = def->parts[i].coll_id;
-		else
+		} else {
 			part->coll_id = COLL_NONE;
+			part->type = FIELD_TYPE_SCALAR;
+		}
 	}
 	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
 						      field_count);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index beaafe1bc..e51e2db2a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -492,7 +492,6 @@ sql_affinity_to_field_type(enum affinity_type affinity)
 		case AFFINITY_INTEGER:
 			return FIELD_TYPE_INTEGER;
 		case AFFINITY_REAL:
-		case AFFINITY_NUMERIC:
 			return FIELD_TYPE_NUMBER;
 		case AFFINITY_TEXT:
 			return FIELD_TYPE_STRING;
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index b67b22c23..7a0b929a7 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -265,7 +265,7 @@ sql_affinity_result(enum affinity_type aff1, enum affinity_type aff2)
 		 */
 		if (sqlite3IsNumericAffinity(aff1)
 		    || sqlite3IsNumericAffinity(aff2)) {
-			return AFFINITY_NUMERIC;
+			return AFFINITY_REAL;
 		} else {
 			return AFFINITY_BLOB;
 		}
@@ -2168,12 +2168,10 @@ sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
 		op = p->op2;
 	switch (op) {
 	case TK_INTEGER:{
-			return aff == AFFINITY_INTEGER
-			    || aff == AFFINITY_NUMERIC;
+			return aff == AFFINITY_INTEGER;
 		}
 	case TK_FLOAT:{
-			return aff == AFFINITY_REAL
-			    || aff == AFFINITY_NUMERIC;
+			return aff == AFFINITY_REAL;
 		}
 	case TK_STRING:{
 			return aff == AFFINITY_TEXT;
@@ -2185,7 +2183,7 @@ sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
 			assert(p->iTable >= 0);	/* p cannot be part of a CHECK constraint */
 			return p->iColumn < 0
 			    && (aff == AFFINITY_INTEGER
-				|| aff == AFFINITY_NUMERIC);
+				|| aff == AFFINITY_REAL);
 		}
 	default:{
 			return 0;
@@ -3700,6 +3698,10 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				sqlite3VdbeAddOp3(v, OP_Column,
 						  pAggInfo->sortingIdxPTab,
 						  pCol->iSorterColumn, target);
+				if (pCol->space_def->fields[pExpr->iAgg].type ==
+				    FIELD_TYPE_NUMBER)
+					sqlite3VdbeAddOp1(v, OP_RealAffinity,
+							  target);
 				return target;
 			}
 			/* Otherwise, fall thru into the TK_COLUMN case */
@@ -3885,14 +3887,14 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			testcase(regFree1 == 0);
 			testcase(regFree2 == 0);
 			if (op != TK_CONCAT)
-				pExpr->affinity = AFFINITY_NUMERIC;
+				pExpr->affinity = AFFINITY_REAL;
 			else
 				pExpr->affinity = AFFINITY_TEXT;
 			break;
 		}
 	case TK_UMINUS:{
 			Expr *pLeft = pExpr->pLeft;
-			pExpr->affinity = AFFINITY_NUMERIC;
+			pExpr->affinity = AFFINITY_REAL;
 			assert(pLeft);
 			if (pLeft->op == TK_INTEGER) {
 				expr_code_int(pParse, pLeft, true, target);
@@ -4184,7 +4186,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 						     target);
 		}
 	case TK_UPLUS:{
-			pExpr->affinity = AFFINITY_NUMERIC;
+			pExpr->affinity = AFFINITY_REAL;
 			return sqlite3ExprCodeTarget(pParse, pExpr->pLeft,
 						     target);
 		}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 02ee225f1..40336e679 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1427,6 +1427,8 @@ sql_expr_list_to_key_info(struct Parse *parse, struct ExprList *list, int start)
 		sql_expr_coll(parse, item->pExpr, &unused, &id);
 		part->coll_id = id;
 		part->sort_order = item->sort_order;
+		enum affinity_type aff = sqlite3ExprAffinity(item->pExpr);
+		part->type =sql_affinity_to_field_type(aff);
 	}
 	return key_info;
 }
@@ -1723,7 +1725,6 @@ generateColumnNames(Parse * pParse,	/* Parser context */
 					      SQLITE_TRANSIENT);
 			break;
 		case AFFINITY_REAL:
-		case AFFINITY_NUMERIC:
 			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC",
 					      SQLITE_TRANSIENT);
 			break;
@@ -5789,8 +5790,11 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 	/* If the output is destined for a temporary table, open that table.
 	 */
 	if (pDest->eDest == SRT_EphemTab) {
-		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, pDest->reg_eph,
-				  pEList->nExpr + 1);
+		struct sql_key_info *key_info =
+			sql_expr_list_to_key_info(pParse, pEList, 0);
+		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, pDest->reg_eph,
+				  pEList->nExpr + 1, 0, (char *)key_info,
+				  P4_KEYINFO);
 		sqlite3VdbeAddOp3(v, OP_IteratorOpen, pDest->iSDParm, 0,
 				  pDest->reg_eph);
 
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index b7403d650..e2d630929 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1788,7 +1788,7 @@ struct Savepoint {
 #define SAVEPOINT_RELEASE    1
 #define SAVEPOINT_ROLLBACK   2
 
-#define sqlite3IsNumericAffinity(X)  ((X)>=AFFINITY_NUMERIC)
+#define sqlite3IsNumericAffinity(X)  ((X)>=AFFINITY_INTEGER)
 
 /*
  * The AFFINITY_MASK values masks off the significant bits of an
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e6b413c70..4345af24e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -310,7 +310,6 @@ applyNumericAffinity(Mem *pRec, int bTryForInt)
  *
  * AFFINITY_INTEGER:
  * AFFINITY_REAL:
- * AFFINITY_NUMERIC:
  *    Try to convert mem to an integer representation or a
  *    floating-point representation if an integer representation
  *    is not possible.  Note that the integer representation is
@@ -345,13 +344,9 @@ mem_apply_affinity(struct Mem *record, enum affinity_type affinity)
 		}
 		return sqlite3VdbeMemIntegerify(record, false);
 	case AFFINITY_REAL:
-		if ((record->flags & MEM_Real) == MEM_Real)
-			return 0;
-		return sqlite3VdbeMemRealify(record);
-	case AFFINITY_NUMERIC:
 		if ((record->flags & (MEM_Real | MEM_Int)) != 0)
 			return 0;
-		return sqlite3VdbeMemNumerify(record);
+		return sqlite3VdbeMemRealify(record);
 	case AFFINITY_TEXT:
 		/*
 		 * Only attempt the conversion to TEXT if there is
@@ -2005,7 +2000,6 @@ case OP_Cast: {                  /* in1 */
 	assert(pOp->p2>=AFFINITY_BLOB && pOp->p2<=AFFINITY_REAL);
 	testcase( pOp->p2==AFFINITY_TEXT);
 	testcase( pOp->p2==AFFINITY_BLOB);
-	testcase( pOp->p2==AFFINITY_NUMERIC);
 	testcase( pOp->p2==AFFINITY_INTEGER);
 	testcase( pOp->p2==AFFINITY_REAL);
 	pIn1 = &aMem[pOp->p1];
@@ -2173,7 +2167,7 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 	} else {
 		/* Neither operand is NULL.  Do a comparison. */
 		affinity = pOp->p5 & AFFINITY_MASK;
-		if (affinity>=AFFINITY_NUMERIC) {
+		if (affinity>=AFFINITY_INTEGER) {
 			if ((flags1 | flags3)&MEM_Str) {
 				if ((flags1 & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
 					applyNumericAffinity(pIn1,0);
@@ -2768,6 +2762,22 @@ case OP_Column: {
 		pDest->flags = MEM_Blob|MEM_Ephem|MEM_Subtype;
 		pDest->subtype = SQL_SUBTYPE_MSGPACK;
 	}
+	if ((pDest->flags & MEM_Int) != 0 &&
+	    pC->eCurType == CURTYPE_TARANTOOL) {
+		enum field_type f = FIELD_TYPE_ANY;
+		/*
+		 * Ephemeral spaces feature only one index
+		 * covering all fields, but ephemeral spaces
+		 * lack format. So, we can fetch type from
+		 * key parts.
+		 */
+		if (pC->uc.pCursor->curFlags & BTCF_TEphemCursor)
+			f = pC->uc.pCursor->index->def->key_def->parts[p2].type;
+		else if (pC->uc.pCursor->curFlags & BTCF_TaCursor)
+			f = pC->uc.pCursor->space->def->fields[p2].type;
+		if (f == FIELD_TYPE_NUMBER)
+			sqlite3VdbeMemSetDouble(pDest, pDest->u.i);
+	}
 	/*
 	 * Add 0 termination (at most for strings)
 	 * Not sure why do we check MEM_Ephem
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index fc805e3aa..08eaddbfc 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3561,17 +3561,30 @@ sqlite3VdbeMsgpackRecordPut(u8 * pBuf, Mem * pRec, u32 n)
 	assert(n != 0);
 	do {
 		assert(memIsValid(pRec));
+		int64_t i;
 		if (pRec->flags & MEM_Null) {
 			zNewRecord = mp_encode_nil(zNewRecord);
 		} else if (pRec->flags & MEM_Real) {
+			/*
+			 * We can't pass to INT iterator float
+			 * value. Hence, if floating point value
+			 * lacks fractional component, we can
+			 * encode it as INT and successfully
+			 * pass to INT iterator.
+			 */
+			i = pRec->u.r;
+			if (i == pRec->u.r)
+				goto encode_int;
 			zNewRecord = mp_encode_double(zNewRecord, pRec->u.r);
 		} else if (pRec->flags & MEM_Int) {
-			if (pRec->u.i >= 0) {
+			i = pRec->u.i;
+encode_int:
+			if (i >= 0) {
 				zNewRecord =
-				    mp_encode_uint(zNewRecord, pRec->u.i);
+				    mp_encode_uint(zNewRecord, i);
 			} else {
 				zNewRecord =
-				    mp_encode_int(zNewRecord, pRec->u.i);
+				    mp_encode_int(zNewRecord, i);
 			}
 		} else if (pRec->flags & MEM_Str) {
 			zNewRecord =
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 22beba8be..bb4d91aed 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -602,8 +602,7 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff)
 {
 	if (pMem->flags & MEM_Null)
 		return SQLITE_OK;
-	if ((pMem->flags & MEM_Blob) != 0 &&
-	    (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) {
+	if ((pMem->flags & MEM_Blob) != 0 && aff == AFFINITY_REAL) {
 		if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, pMem->n) == 0) {
 			MemSetTypeFlag(pMem, MEM_Real);
 			pMem->u.r = pMem->u.i;
@@ -626,8 +625,6 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff)
 			return 0;
 		}
 		return SQLITE_ERROR;
-	case AFFINITY_NUMERIC:
-		return sqlite3VdbeMemNumerify(pMem);
 	case AFFINITY_INTEGER:
 		if ((pMem->flags & MEM_Blob) != 0) {
 			if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i,
@@ -1319,7 +1316,7 @@ valueFromExpr(sqlite3 * db,	/* The database connection */
 		}
 		if ((op == TK_INTEGER || op == TK_FLOAT)
 		    && affinity == AFFINITY_BLOB) {
-			sqlite3ValueApplyAffinity(pVal, AFFINITY_NUMERIC);
+			sqlite3ValueApplyAffinity(pVal, AFFINITY_REAL);
 		} else {
 			sqlite3ValueApplyAffinity(pVal, affinity);
 		}
diff --git a/test/sql-tap/tkt-80e031a00f.test.lua b/test/sql-tap/tkt-80e031a00f.test.lua
index 0cbdb47a2..2d4f81798 100755
--- a/test/sql-tap/tkt-80e031a00f.test.lua
+++ b/test/sql-tap/tkt-80e031a00f.test.lua
@@ -346,7 +346,7 @@ test:do_catchsql_test(
         SELECT 'hello' IN t1
     ]], {
         -- <tkt-80e031a00f.27>
-        1, 'Type mismatch: can not convert hello to numeric'
+        1, 'Type mismatch: can not convert hello to real'
         -- </tkt-80e031a00f.27>
     })
 
@@ -356,7 +356,7 @@ test:do_catchsql_test(
         SELECT 'hello' NOT IN t1
     ]], {
         -- <tkt-80e031a00f.28>
-        1, 'Type mismatch: can not convert hello to numeric'
+        1, 'Type mismatch: can not convert hello to real'
         -- </tkt-80e031a00f.28>
     })
 
@@ -380,23 +380,23 @@ test:do_execsql_test(
         -- </tkt-80e031a00f.30>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "tkt-80e031a00f.31",
     [[
         SELECT x'303132' IN t1
     ]], {
         -- <tkt-80e031a00f.31>
-        0
+        1, 'Type mismatch: can not convert 012 to real'
         -- </tkt-80e031a00f.31>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "tkt-80e031a00f.32",
     [[
         SELECT x'303132' NOT IN t1
     ]], {
         -- <tkt-80e031a00f.32>
-        1
+        1, 'Type mismatch: can not convert 012 to real'
         -- </tkt-80e031a00f.32>
     })
 
-- 
2.15.1