[Tarantool-patches] [PATCH v1 5/7] sql: remove implicit cast from OP_MakeRecord

imeevma at tarantool.org imeevma at tarantool.org
Wed Jul 28 23:51:16 MSK 2021


This patch removes deprecated implicit cast from OP_MakeRecord opcode.
Not there will be no implicit cast in OP_MakeRecord opcode.

Closes #4230
Part of #4470
---
 .../gh-4230-implicit-cast-for-comparison.md   |  3 +++
 src/box/sql/analyze.c                         |  7 +------
 src/box/sql/delete.c                          |  8 ++------
 src/box/sql/expr.c                            |  9 ++-------
 src/box/sql/fk_constraint.c                   |  9 ++-------
 src/box/sql/insert.c                          | 14 --------------
 src/box/sql/sqlInt.h                          |  4 ----
 src/box/sql/update.c                          | 14 +++-----------
 src/box/sql/vdbe.c                            | 19 +------------------
 src/box/sql/wherecode.c                       | 15 +--------------
 test/sql-tap/cast.test.lua                    | 11 ++++++++++-
 test/sql-tap/in3.test.lua                     |  4 ++--
 12 files changed, 27 insertions(+), 90 deletions(-)
 create mode 100644 changelogs/unreleased/gh-4230-implicit-cast-for-comparison.md

diff --git a/changelogs/unreleased/gh-4230-implicit-cast-for-comparison.md b/changelogs/unreleased/gh-4230-implicit-cast-for-comparison.md
new file mode 100644
index 000000000..9f523d3ed
--- /dev/null
+++ b/changelogs/unreleased/gh-4230-implicit-cast-for-comparison.md
@@ -0,0 +1,3 @@
+## feature/sql
+
+* Implicit cast for comparison now works according to defined rules (gh-4230).
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index b87f69512..e97fefb3a 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -968,12 +968,7 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
 		/* Add the entry to the stat1 table. */
 		callStatGet(v, stat4_reg, STAT_GET_STAT1, stat1_reg);
-		enum field_type types[4] = { FIELD_TYPE_STRING,
-					     FIELD_TYPE_STRING,
-					     FIELD_TYPE_STRING,
-					     field_type_MAX };
-		sqlVdbeAddOp4(v, OP_MakeRecord, tab_name_reg, 4, tmp_reg,
-				  (char *)types, sizeof(types));
+		sqlVdbeAddOp3(v, OP_MakeRecord, tab_name_reg, 4, tmp_reg);
 		sqlVdbeAddOp4(v, OP_IdxInsert, tmp_reg, 0, 0,
 				  (char *)stat1, P4_SPACEPTR);
 		/* Add the entries to the stat4 table. */
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 62a726fdd..5226dd6ea 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -328,12 +328,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			 * key.
 			 */
 			key_len = 0;
-			struct index *pk = space_index(space, 0);
-			enum field_type *types = is_view ? NULL :
-						 sql_index_type_str(parse->db,
-								    pk->def);
-			sqlVdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
-					  reg_key, (char *)types, P4_DYNAMIC);
+			sqlVdbeAddOp3(v, OP_MakeRecord, reg_pk, pk_len,
+				      reg_key);
 			/* Set flag to save memory allocating one
 			 * by malloc.
 			 */
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3772596d6..8635f2443 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2859,8 +2859,6 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 				struct ExprList_item *pItem;
 				int r1, r2, r3;
 
-				enum field_type lhs_type =
-					sql_expr_type(pLeft);
 				bool unused;
 				struct coll *unused_coll;
 				if (sql_expr_coll(pParse, pExpr->pLeft, &unused,
@@ -2886,11 +2884,8 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 						jmpIfDynamic = -1;
 					}
 					r3 = sqlExprCodeTarget(pParse, pE2, r1);
-					enum field_type types[2] =
-						{ lhs_type, field_type_MAX };
-	 				sqlVdbeAddOp4(v, OP_MakeRecord, r3,
-							  1, r2, (char *)types,
-							  sizeof(types));
+					sqlVdbeAddOp3(v, OP_MakeRecord, r3, 1,
+						      r2);
 					sql_expr_type_cache_change(pParse,
 								   r3, 1);
 					sqlVdbeAddOp2(v, OP_IdxInsert, r2,
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 0dd10c420..2a9399d78 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -262,13 +262,8 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 					  link->child_field + 1 + reg_data,
 					  temp_regs + i);
 		}
-		struct index *idx = space_index(parent, referenced_idx);
-		assert(idx != NULL);
-		sqlVdbeAddOp4(v, OP_MakeRecord, temp_regs, field_count,
-				  rec_reg,
-				  (char *) sql_index_type_str(parse_context->db,
-							      idx->def),
-				  P4_DYNAMIC);
+		sqlVdbeAddOp3(v, OP_MakeRecord, temp_regs, field_count,
+			      rec_reg);
 		sqlVdbeAddOp4Int(v, OP_Found, cursor, ok_label, rec_reg, 0);
 		sqlReleaseTempReg(parse_context, rec_reg);
 		sqlReleaseTempRange(parse_context, temp_regs, field_count);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 02e9f9673..21b4f2407 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -42,20 +42,6 @@
 #include "box/box.h"
 #include "box/schema.h"
 
-enum field_type *
-sql_index_type_str(struct sql *db, const struct index_def *idx_def)
-{
-	uint32_t column_count = idx_def->key_def->part_count;
-	uint32_t sz = (column_count + 1) * sizeof(enum field_type);
-	enum field_type *types = (enum field_type *) sqlDbMallocRaw(db, sz);
-	if (types == NULL)
-		return NULL;
-	for (uint32_t i = 0; i < column_count; i++)
-		types[i] = idx_def->key_def->parts[i].type;
-	types[column_count] = field_type_MAX;
-	return types;
-}
-
 void
 sql_emit_table_types(struct Vdbe *v, struct space_def *def, int reg)
 {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 115c52f96..4f94afdf7 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3792,10 +3792,6 @@ int sqlVarintLen(u64 v);
 #define getVarint    sqlGetVarint
 #define putVarint    sqlPutVarint
 
-/** Return string consisting of fields types of given index. */
-enum field_type *
-sql_index_type_str(struct sql *db, const struct index_def *idx_def);
-
 /**
  * Code an OP_ApplyType opcode that will force types
  * for given range of register starting from @reg.
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 24c7cfa27..22f82390c 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -251,11 +251,7 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 		nKey = pk_part_count;
 		regKey = iPk;
 	} else {
-		enum field_type *types = is_view ? NULL :
-					 sql_index_type_str(pParse->db,
-							    pPk->def);
-		sqlVdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count,
-				  regKey, (char *) types, P4_DYNAMIC);
+		sqlVdbeAddOp3(v, OP_MakeRecord, iPk, pk_part_count, regKey);
 		/*
 		 * Set flag to save memory allocating one by
 		 * malloc.
@@ -420,12 +416,8 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 			int key_reg;
 			if (okOnePass) {
 				key_reg = sqlGetTempReg(pParse);
-				enum field_type *types =
-					sql_index_type_str(pParse->db,
-							   pPk->def);
-				sqlVdbeAddOp4(v, OP_MakeRecord, iPk,
-						  pk_part_count, key_reg,
-						  (char *) types, P4_DYNAMIC);
+				sqlVdbeAddOp3(v, OP_MakeRecord, iPk,
+					      pk_part_count, key_reg);
 			} else {
 				assert(nKey == 0);
 				key_reg = regKey;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c24265a1d..81551acf3 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2051,24 +2051,17 @@ case OP_ApplyType: {
 	break;
 }
 
-/* Opcode: MakeRecord P1 P2 P3 P4 P5
+/* Opcode: MakeRecord P1 P2 P3 * P5
  * Synopsis: r[P3]=mkrec(r[P1 at P2])
  *
  * Convert P2 registers beginning with P1 into the [record format]
  * use as a data record in a database table or as a key
  * in an index.  The OP_Column opcode can decode the record later.
  *
- * P4 may be a string that is P2 characters long.  The nth character of the
- * string indicates the column type that should be used for the nth
- * field of the index key.
- *
- * If P4 is NULL then all index fields have type SCALAR.
- *
  * If P5 is not NULL then record under construction is intended to be inserted
  * into ephemeral space. Thus, sort of memory optimization can be performed.
  */
 case OP_MakeRecord: {
-	Mem *pRec;             /* The new record */
 	Mem *pData0;           /* First field to be combined into the record */
 	Mem MAYBE_UNUSED *pLast;  /* Last field of the record */
 	int nField;            /* Number of fields in the record */
@@ -2089,7 +2082,6 @@ case OP_MakeRecord: {
 	 * is the offset from the beginning of the record to data0.
 	 */
 	nField = pOp->p1;
-	enum field_type *types = pOp->p4.types;
 	bIsEphemeral = pOp->p5;
 	assert(nField>0 && pOp->p2>0 && pOp->p2+nField<=(p->nMem+1 - p->nCursor)+1);
 	pData0 = &aMem[nField];
@@ -2100,15 +2092,6 @@ case OP_MakeRecord: {
 	assert(pOp->p3<pOp->p1 || pOp->p3>=pOp->p1+pOp->p2);
 	pOut = vdbe_prepare_null_out(p, pOp->p3);
 
-	/* Apply the requested types to all inputs */
-	assert(pData0<=pLast);
-	if (types != NULL) {
-		pRec = pData0;
-		do {
-			mem_cast_implicit_old(pRec++, *(types++));
-		} while(types[0] != field_type_MAX);
-	}
-
 	struct region *region = &fiber()->gc;
 	size_t used = region_used(region);
 	uint32_t tuple_size;
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 92d374200..df6cc92e1 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -602,9 +602,6 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
 	nReg = pLoop->nEq + nExtraReg;
 	pParse->nMem += nReg;
 
-	enum field_type *type = sql_index_type_str(pParse->db, idx_def);
-	assert(type != NULL || pParse->db->mallocFailed);
-
 	if (nSkip) {
 		int iIdxCur = pLevel->iIdxCur;
 		sqlVdbeAddOp1(v, (bRev ? OP_Last : OP_Rewind), iIdxCur);
@@ -647,17 +644,7 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
 				sqlVdbeAddOp2(v, OP_SCopy, r1, regBase + j);
 			}
 		}
-		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;
-			}
-		} else if ((pTerm->eOperator & WO_ISNULL) == 0) {
+		if ((pTerm->eOperator & (WO_IN | WO_ISNULL)) == 0) {
 			Expr *pRight = pTerm->pExpr->pRight;
 			if (sqlExprCanBeNull(pRight)) {
 				sqlVdbeAddOp2(v, OP_IsNull, regBase + j,
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index b570fc878..927114772 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(108)
+test:plan(109)
 
 --!./tcltestrunner.lua
 -- 2005 June 25
@@ -1156,4 +1156,13 @@ test:do_execsql_test(
         10000000000000000
     })
 
+-- Make sure that there is no unnecessary implicit casts in IN operator.
+test:do_catchsql_test(
+    "cast-12",
+    [[
+        SELECT 1 IN (SELECT '1');
+    ]], {
+        1, "Type mismatch: can not convert integer(1) to string"
+    })
+
 test:finish_test()
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index 5f3f543af..4536fb0d3 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -342,7 +342,7 @@ test:do_test(
         return exec_neph(" SELECT y IN (SELECT a FROM t1) FROM t2 ")
     end, {
         -- <in3-3.5>
-        1, true
+        1, false
         -- </in3-3.5>
     })
 
@@ -366,7 +366,7 @@ test:do_test(
         return exec_neph(" SELECT y IN (SELECT c FROM t1) FROM t2 ")
     end, {
         -- <in3-3.7>
-        1, true
+        1, false
         -- </in3-3.7>
     })
 
-- 
2.25.1



More information about the Tarantool-patches mailing list