[Tarantool-patches] [PATCH v1 5/7] sql: remove implicit cast from OP_MakeRecord
Mergen Imeev
imeevma at tarantool.org
Fri Aug 6 02:43:19 MSK 2021
Thank you for the review! My answers and new patch below.
On Thu, Aug 05, 2021 at 12:27:20AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> On 28.07.2021 22:51, imeevma at tarantool.org wrote:
> > This patch removes deprecated implicit cast from OP_MakeRecord opcode.
> > Not there will be no implicit cast in OP_MakeRecord opcode.
>
> 1. Not -> Note? The commit seems strange. You basically said the same
> thing 3 times: 1 in th title and 2 in the message.
>
"Not" was "Now", but I rewrite all commite-message.
> > 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).
>
> 2. Please, try to be more specific here. Users won't understand anything from
> this sentence.
Fixed.
New patch:
commit 71f945038ee7aca90e854dad29875312adf80a1a
Author: Mergen Imeev <imeevma at gmail.com>
Date: Tue Jul 27 23:31:45 2021 +0300
sql: remove implicit cast from OP_MakeRecord
This patch removes deprecated implicit cast from OP_MakeRecord opcode,
which were used in some rare cases, for example during IN operation
with subselect as right-value.
Closes #4230
Part of #4470
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..66103f461
--- /dev/null
+++ b/changelogs/unreleased/gh-4230-implicit-cast-for-comparison.md
@@ -0,0 +1,6 @@
+## feature/sql
+
+* Now any number can be compared to any other number, and values of any scalar
+ type can be compared to any other value of the same type. A value of a
+ non-numeric scalar type cannot be compared with a value of any other scalar
+ type (gh-4230).
\ No newline at end of file
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 b9fc5bfc5..83766b3fb 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2829,8 +2829,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,
@@ -2856,11 +2854,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 6b3444cf3..adac80a64 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3771,10 +3771,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 aedff8b09..fcea9eefe 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2049,24 +2049,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 */
@@ -2087,7 +2080,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];
@@ -2098,15 +2090,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>
})
More information about the Tarantool-patches
mailing list