This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and may cause errors. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify @ChangeLog - Fixed a bug with unnecessary conversation from INTEGER to DOUBLE (gh-5335). src/box/sql/delete.c | 12 ----- src/box/sql/expr.c | 13 ----- src/box/sql/vdbe.c | 17 ------- ...-unnecessary-conversation-to-double.result | 51 +++++++++++++++++++ ...nnecessary-conversation-to-double.test.lua | 11 ++++ 5 files changed, 62 insertions(+), 42 deletions(-) create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.result create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.test.lua diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index 68abd1f58..a78c68df6 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -565,18 +565,6 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor, } uint32_t tabl_col = index->def->key_def->parts[j].fieldno; sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j); - /* - * If the column type is NUMBER but the number - * is an integer, then it might be stored in the - * table as an integer (using a compact - * representation) then converted to REAL by an - * OP_Realify opcode. But we are getting - * ready to store this value back into an index, - * where it should be converted by to INTEGER - * again. So omit the OP_Realify opcode if - * it is present - */ - sqlVdbeDeletePriorOpcode(v, OP_Realify); } if (reg_out != 0) sqlVdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt, reg_out); diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index bc2182446..09a2d3ef9 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* Otherwise, fall thru into the TK_COLUMN case */ @@ -4257,14 +4252,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 14ddb5160..a86461c4c 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2105,23 +2105,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if ((pIn1->flags & (MEM_Int | MEM_UInt)) != 0) { - sqlVdbeMemRealify(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql/gh-5335-unnecessary-conversation-to-double.result b/test/sql/gh-5335-unnecessary-conversation-to-double.result new file mode 100644 index 000000000..e3f0f0b0b --- /dev/null +++ b/test/sql/gh-5335-unnecessary-conversation-to-double.result @@ -0,0 +1,51 @@ +-- test-run result file version 2 +-- Make sure that INTEGER is not converted to DOUBLE in cases below. +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") + | --- + | - row_count: 1 + | ... +box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") + | --- + | - row_count: 1 + | ... +box.execute("INSERT INTO t VALUES (1, 1);") + | --- + | - row_count: 1 + | ... +box.execute("SELECT i / 2, n / 2 FROM t;") + | --- + | - metadata: + | - name: COLUMN_1 + | type: number + | - name: COLUMN_2 + | type: number + | rows: + | - [0, 0] + | ... +box.execute("DROP TABLE t;") + | --- + | - row_count: 1 + | ... + +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") + | --- + | - row_count: 1 + | ... +box.execute("INSERT INTO t VALUES (1,1);") + | --- + | - row_count: 1 + | ... +box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") + | --- + | - metadata: + | - name: COLUMN_1 + | type: number + | - name: COLUMN_2 + | type: number + | rows: + | - [0, 0] + | ... +box.execute("DROP TABLE t;") + | --- + | - row_count: 1 + | ... diff --git a/test/sql/gh-5335-unnecessary-conversation-to-double.test.lua b/test/sql/gh-5335-unnecessary-conversation-to-double.test.lua new file mode 100644 index 000000000..1663d054a --- /dev/null +++ b/test/sql/gh-5335-unnecessary-conversation-to-double.test.lua @@ -0,0 +1,11 @@ +-- Make sure that INTEGER is not converted to DOUBLE in cases below. +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") +box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") +box.execute("INSERT INTO t VALUES (1, 1);") +box.execute("SELECT i / 2, n / 2 FROM t;") +box.execute("DROP TABLE t;") + +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") +box.execute("INSERT INTO t VALUES (1,1);") +box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") +box.execute("DROP TABLE t;") -- 2.25.1
Hi! Thanks for the patch! LGTM.
This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and may cause errors. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify @ChangeLog - Fixed a bug with unnecessary convertion from INTEGER to DOUBLE (gh-5335). src/box/sql/delete.c | 12 ----- src/box/sql/expr.c | 13 ----- src/box/sql/vdbe.c | 17 ------- ...-unnecessary-conversation-to-double.result | 51 +++++++++++++++++++ ...nnecessary-conversation-to-double.test.lua | 11 ++++ 5 files changed, 62 insertions(+), 42 deletions(-) create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.result create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.test.lua diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index 68abd1f58..a78c68df6 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -565,18 +565,6 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor, } uint32_t tabl_col = index->def->key_def->parts[j].fieldno; sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j); - /* - * If the column type is NUMBER but the number - * is an integer, then it might be stored in the - * table as an integer (using a compact - * representation) then converted to REAL by an - * OP_Realify opcode. But we are getting - * ready to store this value back into an index, - * where it should be converted by to INTEGER - * again. So omit the OP_Realify opcode if - * it is present - */ - sqlVdbeDeletePriorOpcode(v, OP_Realify); } if (reg_out != 0) sqlVdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt, reg_out); diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index bc2182446..09a2d3ef9 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* Otherwise, fall thru into the TK_COLUMN case */ @@ -4257,14 +4252,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 14ddb5160..a86461c4c 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2105,23 +2105,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if ((pIn1->flags & (MEM_Int | MEM_UInt)) != 0) { - sqlVdbeMemRealify(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql/gh-5335-unnecessary-conversation-to-double.result b/test/sql/gh-5335-unnecessary-conversation-to-double.result new file mode 100644 index 000000000..e3f0f0b0b --- /dev/null +++ b/test/sql/gh-5335-unnecessary-conversation-to-double.result @@ -0,0 +1,51 @@ +-- test-run result file version 2 +-- Make sure that INTEGER is not converted to DOUBLE in cases below. +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") + | --- + | - row_count: 1 + | ... +box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") + | --- + | - row_count: 1 + | ... +box.execute("INSERT INTO t VALUES (1, 1);") + | --- + | - row_count: 1 + | ... +box.execute("SELECT i / 2, n / 2 FROM t;") + | --- + | - metadata: + | - name: COLUMN_1 + | type: number + | - name: COLUMN_2 + | type: number + | rows: + | - [0, 0] + | ... +box.execute("DROP TABLE t;") + | --- + | - row_count: 1 + | ... + +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") + | --- + | - row_count: 1 + | ... +box.execute("INSERT INTO t VALUES (1,1);") + | --- + | - row_count: 1 + | ... +box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") + | --- + | - metadata: + | - name: COLUMN_1 + | type: number + | - name: COLUMN_2 + | type: number + | rows: + | - [0, 0] + | ... +box.execute("DROP TABLE t;") + | --- + | - row_count: 1 + | ... diff --git a/test/sql/gh-5335-unnecessary-conversation-to-double.test.lua b/test/sql/gh-5335-unnecessary-conversation-to-double.test.lua new file mode 100644 index 000000000..1663d054a --- /dev/null +++ b/test/sql/gh-5335-unnecessary-conversation-to-double.test.lua @@ -0,0 +1,11 @@ +-- Make sure that INTEGER is not converted to DOUBLE in cases below. +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") +box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") +box.execute("INSERT INTO t VALUES (1, 1);") +box.execute("SELECT i / 2, n / 2 FROM t;") +box.execute("DROP TABLE t;") + +box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") +box.execute("INSERT INTO t VALUES (1,1);") +box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") +box.execute("DROP TABLE t;") -- 2.25.1
On 26 Sep 17:15, imeevma@tarantool.org wrote: > This opcode was used to convert INTEGER values to REAL. It is not > necessary in Tarantool and may cause errors. Add justification: what this change fixes, in which way etc. > Closes #5335 > --- > https://github.com/tarantool/tarantool/issues/5335 > https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify > > @ChangeLog > - Fixed a bug with unnecessary convertion from INTEGER to DOUBLE (gh-5335). > > src/box/sql/delete.c | 12 ----- > src/box/sql/expr.c | 13 ----- > src/box/sql/vdbe.c | 17 ------- > ...-unnecessary-conversation-to-double.result | 51 +++++++++++++++++++ > ...nnecessary-conversation-to-double.test.lua | 11 ++++ > 5 files changed, 62 insertions(+), 42 deletions(-) > create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.result > create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.test.lua > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 68abd1f58..a78c68df6 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -565,18 +565,6 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor, > } > uint32_t tabl_col = index->def->key_def->parts[j].fieldno; > sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j); > - /* > - * If the column type is NUMBER but the number > - * is an integer, then it might be stored in the > - * table as an integer (using a compact > - * representation) then converted to REAL by an > - * OP_Realify opcode. But we are getting > - * ready to store this value back into an index, > - * where it should be converted by to INTEGER > - * again. So omit the OP_Realify opcode if > - * it is present > - */ > - sqlVdbeDeletePriorOpcode(v, OP_Realify); > } > if (reg_out != 0) > sqlVdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt, reg_out);
Hi! Thank you for review! My answer below. On Mon, Sep 28, 2020 at 03:55:59PM +0000, Nikita Pettik wrote: > On 26 Sep 17:15, imeevma@tarantool.org wrote: > > This opcode was used to convert INTEGER values to REAL. It is not > > necessary in Tarantool and may cause errors. > > Add justification: what this change fixes, in which way etc. > Fixed: commit 379694944e39a3b0c33135a724f2ee246c619110 Author: Mergen Imeev <imeevma@gmail.com> Date: Sat Sep 26 12:43:18 2020 +0300 sql: remove OP_Realify This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 > > Closes #5335 > > --- > > https://github.com/tarantool/tarantool/issues/5335 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify > > > > @ChangeLog > > - Fixed a bug with unnecessary convertion from INTEGER to DOUBLE (gh-5335). > > > > src/box/sql/delete.c | 12 ----- > > src/box/sql/expr.c | 13 ----- > > src/box/sql/vdbe.c | 17 ------- > > ...-unnecessary-conversation-to-double.result | 51 +++++++++++++++++++ > > ...nnecessary-conversation-to-double.test.lua | 11 ++++ > > 5 files changed, 62 insertions(+), 42 deletions(-) > > create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.result > > create mode 100644 test/sql/gh-5335-unnecessary-conversation-to-double.test.lua > > > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > > index 68abd1f58..a78c68df6 100644 > > --- a/src/box/sql/delete.c > > +++ b/src/box/sql/delete.c > > @@ -565,18 +565,6 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor, > > } > > uint32_t tabl_col = index->def->key_def->parts[j].fieldno; > > sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j); > > - /* > > - * If the column type is NUMBER but the number > > - * is an integer, then it might be stored in the > > - * table as an integer (using a compact > > - * representation) then converted to REAL by an > > - * OP_Realify opcode. But we are getting > > - * ready to store this value back into an index, > > - * where it should be converted by to INTEGER > > - * again. So omit the OP_Realify opcode if > > - * it is present > > - */ > > - sqlVdbeDeletePriorOpcode(v, OP_Realify); > > } > > if (reg_out != 0) > > sqlVdbeAddOp3(v, OP_MakeRecord, reg_base, col_cnt, reg_out);
This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify src/box/sql/expr.c | 13 ------- src/box/sql/vdbe.c | 17 -------- .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ 3 files changed, 39 insertions(+), 30 deletions(-) create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3772596d6..d2624516c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9e763ed85..0a3c904b1 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if (mem_is_int(pIn1)) { - mem_to_double(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua new file mode 100755 index 000000000..efcae911c --- /dev/null +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -0,0 +1,39 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(2) + +test:execsql([[ + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; + INSERT INTO t1 VALUES (1, 1); + INSERT INTO t2 VALUES (1, 1); +]]) + +-- +-- Make sure that implicit cast from string to integer works correctly in +-- arithmetic operations. +-- +test:do_execsql_test( + "gh-5335-1", + [[ + SELECT i / 2, n / 2 FROM t1; + ]], { + 0, 0 + }) + +test:do_execsql_test( + "gh-5335-2", + [[ + SELECT i / 2, n / 2 FROM t2 GROUP BY n; + ]], { + 0, 0 + }) + +test:execsql([[ + DROP TRIGGER r; + DROP TABLE t1; + DROP TABLE t2; +]]) + +test:finish_test() -- 2.25.1
Thanks for the patch! See 2 comments below. > src/box/sql/expr.c | 13 ------- > src/box/sql/vdbe.c | 17 -------- > .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ 1. Please, add a changelog file. > diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > new file mode 100755 > index 000000000..efcae911c > --- /dev/null > +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > @@ -0,0 +1,39 @@ > +#!/usr/bin/env tarantool > +local test = require("sqltester") > +test:plan(2) > + > +test:execsql([[ > + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); > + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); > + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; > + INSERT INTO t1 VALUES (1, 1); > + INSERT INTO t2 VALUES (1, 1); > +]]) > + > +-- > +-- Make sure that implicit cast from string to integer works correctly in > +-- arithmetic operations. 2. From string to integer? Where are the strings? > +-- > +test:do_execsql_test( > + "gh-5335-1", > + [[ > + SELECT i / 2, n / 2 FROM t1; > + ]], { > + 0, 0 > + }) > + > +test:do_execsql_test( > + "gh-5335-2", > + [[ > + SELECT i / 2, n / 2 FROM t2 GROUP BY n; > + ]], { > + 0, 0 > + }) > + > +test:execsql([[ > + DROP TRIGGER r; > + DROP TABLE t1; > + DROP TABLE t2; > +]]) > + > +test:finish_test() >
Hi! Thank you for the review! My answers, diff and new patch below. On Mon, Jul 26, 2021 at 10:58:05PM +0200, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 2 comments below. > > > src/box/sql/expr.c | 13 ------- > > src/box/sql/vdbe.c | 17 -------- > > .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ > > 1. Please, add a changelog file. > Added. > > diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > > new file mode 100755 > > index 000000000..efcae911c > > --- /dev/null > > +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > > @@ -0,0 +1,39 @@ > > +#!/usr/bin/env tarantool > > +local test = require("sqltester") > > +test:plan(2) > > + > > +test:execsql([[ > > + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); > > + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); > > + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; > > + INSERT INTO t1 VALUES (1, 1); > > + INSERT INTO t2 VALUES (1, 1); > > +]]) > > + > > +-- > > +-- Make sure that implicit cast from string to integer works correctly in > > +-- arithmetic operations. > > 2. From string to integer? Where are the strings? > Fixed. Most likely I forgot to change this comment after I copied tests to use as a template. > > +-- > > +test:do_execsql_test( > > + "gh-5335-1", > > + [[ > > + SELECT i / 2, n / 2 FROM t1; > > + ]], { > > + 0, 0 > > + }) > > + > > +test:do_execsql_test( > > + "gh-5335-2", > > + [[ > > + SELECT i / 2, n / 2 FROM t2 GROUP BY n; > > + ]], { > > + 0, 0 > > + }) > > + > > +test:execsql([[ > > + DROP TRIGGER r; > > + DROP TABLE t1; > > + DROP TABLE t2; > > +]]) > > + > > +test:finish_test() > > Diff: diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md new file mode 100644 index 000000000..b06805a7f --- /dev/null +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type + NUMBER (gh-5335). diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua index efcae911c..d29324a28 100755 --- a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -11,8 +11,8 @@ test:execsql([[ ]]) -- --- Make sure that implicit cast from string to integer works correctly in --- arithmetic operations. +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast in +-- field of type NUMBER. -- test:do_execsql_test( "gh-5335-1", Patch: commit f15e1aefdd7cafd380e66f7b5acdafb7ae5bdca6 Author: Mergen Imeev <imeevma@gmail.com> Date: Sat Sep 26 12:43:18 2020 +0300 sql: remove OP_Realify This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md new file mode 100644 index 000000000..b06805a7f --- /dev/null +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type + NUMBER (gh-5335). diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3772596d6..d2624516c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9e763ed85..0a3c904b1 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if (mem_is_int(pIn1)) { - mem_to_double(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua new file mode 100755 index 000000000..d29324a28 --- /dev/null +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -0,0 +1,39 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(2) + +test:execsql([[ + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; + INSERT INTO t1 VALUES (1, 1); + INSERT INTO t2 VALUES (1, 1); +]]) + +-- +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast in +-- field of type NUMBER. +-- +test:do_execsql_test( + "gh-5335-1", + [[ + SELECT i / 2, n / 2 FROM t1; + ]], { + 0, 0 + }) + +test:do_execsql_test( + "gh-5335-2", + [[ + SELECT i / 2, n / 2 FROM t2 GROUP BY n; + ]], { + 0, 0 + }) + +test:execsql([[ + DROP TRIGGER r; + DROP TABLE t1; + DROP TABLE t2; +]]) + +test:finish_test()
Thanks for the patch! LGTM.
This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify ...gh-5335-remove-wrong-double-to-int-cast.md | 4 ++ src/box/sql/expr.c | 13 ------- src/box/sql/vdbe.c | 17 -------- .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ 4 files changed, 43 insertions(+), 30 deletions(-) create mode 100644 changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md new file mode 100644 index 000000000..b06805a7f --- /dev/null +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type + NUMBER (gh-5335). diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3772596d6..d2624516c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9e763ed85..0a3c904b1 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if (mem_is_int(pIn1)) { - mem_to_double(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua new file mode 100755 index 000000000..d29324a28 --- /dev/null +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -0,0 +1,39 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(2) + +test:execsql([[ + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; + INSERT INTO t1 VALUES (1, 1); + INSERT INTO t2 VALUES (1, 1); +]]) + +-- +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast in +-- field of type NUMBER. +-- +test:do_execsql_test( + "gh-5335-1", + [[ + SELECT i / 2, n / 2 FROM t1; + ]], { + 0, 0 + }) + +test:do_execsql_test( + "gh-5335-2", + [[ + SELECT i / 2, n / 2 FROM t2 GROUP BY n; + ]], { + 0, 0 + }) + +test:execsql([[ + DROP TRIGGER r; + DROP TABLE t1; + DROP TABLE t2; +]]) + +test:finish_test() -- 2.25.1
Sorry. But this is part of a changes, which we have not agreed upon (reminder, that the last agreement with PMs was to get rid of scalar, but keep number arithmetic), and which [I believe] we would have to revert eventually. Please ask Kirill for ack, if he believes that we have to do it for "consistent types" task. Let me clarify how I see this - in this particular case forcing real type semantics for NUMBER field was pretty much consistent behavior. And using integer division on some rows, but double on others is actually making things _much less_ consistent. Regards, : From: imeevma@tarantool.org <imeevma@tarantool.org> : Subject: [PATCH v1 1/1] sql: remove OP_Realify : : This opcode was used to convert INTEGER values to REAL. It is not : necessary in Tarantool and causes errors. : : Due to OP_Realify two type of errors appeared: : 1) In some cases in trigger INTEGER may be converted to DOUBLE. : For example: : box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") : box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t : SET n = new.n; END;") : box.execute("INSERT INTO t VALUES (1, 1);") : box.execute("SELECT i / 2, n / 2 FROM t;") : : Result: : tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") : --- : - metadata: : - name: COLUMN_1 : type: number : - name: COLUMN_2 : type: number : rows: : - [0, 0.5] : ... : : 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. : For example: : box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") : box.execute("INSERT INTO t VALUES (1,1);") : box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") : : Result: : tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") : --- : - metadata: : - name: COLUMN_1 : type: number : - name: COLUMN_2 : type: number : rows: : - [0.5, 0.5] : ... : : This patch removes OP_Realify, after which these errors disappear. : : Closes #5335 : --- : https://github.com/tarantool/tarantool/issues/5335 : https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op- : realify : : ...gh-5335-remove-wrong-double-to-int-cast.md | 4 ++ : src/box/sql/expr.c | 13 ------- : src/box/sql/vdbe.c | 17 -------- : .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ : 4 files changed, 43 insertions(+), 30 deletions(-) : create mode 100644 changelogs/unreleased/gh-5335-remove-wrong-double-to- : int-cast.md : create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua : : diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int- : cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md : new file mode 100644 : index 000000000..b06805a7f : --- /dev/null : +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md : @@ -0,0 +1,4 @@ : +## bugfix/sql : + : +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type : + NUMBER (gh-5335). : diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c : index 3772596d6..d2624516c 100644 : --- a/src/box/sql/expr.c : +++ b/src/box/sql/expr.c : @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int : target) : sqlVdbeAddOp3(v, OP_Column, : pAggInfo->sortingIdxPTab, : pCol->iSorterColumn, target); : - if (pCol->space_def->fields[pExpr->iAgg].type == : - FIELD_TYPE_NUMBER) { : - sqlVdbeAddOp1(v, OP_Realify, : - target); : - } : return target; : } : /* : @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int : target) : (pExpr->iTable ? "new" : "old"), : pExpr->space_def->fields[ : pExpr->iColumn].name, target)); : - : - /* If the column has type NUMBER, it may currently be : stored as an : - * integer. Use OP_Realify to make sure it is really real. : - */ : - if (pExpr->iColumn >= 0 && def->fields[ : - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { : - sqlVdbeAddOp1(v, OP_Realify, target); : - } : break; : } : : diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c : index 9e763ed85..0a3c904b1 100644 : --- a/src/box/sql/vdbe.c : +++ b/src/box/sql/vdbe.c : @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ : break; : } : : -/* Opcode: Realify P1 * * * * : - * : - * If register P1 holds an integer convert it to a real value. : - * : - * This opcode is used when extracting information from a column that : - * has float type. Such column values may still be stored as : - * integers, for space efficiency, but after extraction we want them : - * to have only a real value. : - */ : -case OP_Realify: { /* in1 */ : - pIn1 = &aMem[pOp->p1]; : - if (mem_is_int(pIn1)) { : - mem_to_double(pIn1); : - } : - break; : -} : - : /* Opcode: Cast P1 P2 * * * : * Synopsis: type(r[P1]) : * : diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua : b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua : new file mode 100755 : index 000000000..d29324a28 : --- /dev/null : +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua : @@ -0,0 +1,39 @@ : +#!/usr/bin/env tarantool : +local test = require("sqltester") : +test:plan(2) : + : +test:execsql([[ : + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); : + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); : + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n : = new.n; END; : + INSERT INTO t1 VALUES (1, 1); : + INSERT INTO t2 VALUES (1, 1); : +]]) : + : +-- : +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast : in : +-- field of type NUMBER. : +-- : +test:do_execsql_test( : + "gh-5335-1", : + [[ : + SELECT i / 2, n / 2 FROM t1; : + ]], { : + 0, 0 : + }) : + : +test:do_execsql_test( : + "gh-5335-2", : + [[ : + SELECT i / 2, n / 2 FROM t2 GROUP BY n; : + ]], { : + 0, 0 : + }) : + : +test:execsql([[ : + DROP TRIGGER r; : + DROP TABLE t1; : + DROP TABLE t2; : +]]) : + : +test:finish_test() : -- : 2.25.1
Hi! Thank you for the review! On Fri, Jul 30, 2021 at 10:14:36AM +0300, Timur Safin wrote: > Sorry. But this is part of a changes, which we have not agreed upon (reminder, that the > last agreement with PMs was to get rid of scalar, but keep number arithmetic), and which > [I believe] we would have to revert eventually. Please ask Kirill for ack, if he believes > that we have to do it for "consistent types" task. > > Let me clarify how I see this - in this particular case forcing real type semantics for > NUMBER field was pretty much consistent behavior. And using integer division on some > rows, but double on others is actually making things _much less_ consistent. > > Regards, > Ok, no problem. > > : From: imeevma@tarantool.org <imeevma@tarantool.org> > : Subject: [PATCH v1 1/1] sql: remove OP_Realify > : > : This opcode was used to convert INTEGER values to REAL. It is not > : necessary in Tarantool and causes errors. > : > : Due to OP_Realify two type of errors appeared: > : 1) In some cases in trigger INTEGER may be converted to DOUBLE. > : For example: > : box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") > : box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t > : SET n = new.n; END;") > : box.execute("INSERT INTO t VALUES (1, 1);") > : box.execute("SELECT i / 2, n / 2 FROM t;") > : > : Result: > : tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") > : --- > : - metadata: > : - name: COLUMN_1 > : type: number > : - name: COLUMN_2 > : type: number > : rows: > : - [0, 0.5] > : ... > : > : 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. > : For example: > : box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") > : box.execute("INSERT INTO t VALUES (1,1);") > : box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") > : > : Result: > : tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") > : --- > : - metadata: > : - name: COLUMN_1 > : type: number > : - name: COLUMN_2 > : type: number > : rows: > : - [0.5, 0.5] > : ... > : > : This patch removes OP_Realify, after which these errors disappear. > : > : Closes #5335 > : --- > : https://github.com/tarantool/tarantool/issues/5335 > : https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op- > : realify > : > : ...gh-5335-remove-wrong-double-to-int-cast.md | 4 ++ > : src/box/sql/expr.c | 13 ------- > : src/box/sql/vdbe.c | 17 -------- > : .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ > : 4 files changed, 43 insertions(+), 30 deletions(-) > : create mode 100644 changelogs/unreleased/gh-5335-remove-wrong-double-to- > : int-cast.md > : create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > : > : diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int- > : cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md > : new file mode 100644 > : index 000000000..b06805a7f > : --- /dev/null > : +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md > : @@ -0,0 +1,4 @@ > : +## bugfix/sql > : + > : +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type > : + NUMBER (gh-5335). > : diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > : index 3772596d6..d2624516c 100644 > : --- a/src/box/sql/expr.c > : +++ b/src/box/sql/expr.c > : @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int > : target) > : sqlVdbeAddOp3(v, OP_Column, > : pAggInfo->sortingIdxPTab, > : pCol->iSorterColumn, target); > : - if (pCol->space_def->fields[pExpr->iAgg].type == > : - FIELD_TYPE_NUMBER) { > : - sqlVdbeAddOp1(v, OP_Realify, > : - target); > : - } > : return target; > : } > : /* > : @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int > : target) > : (pExpr->iTable ? "new" : "old"), > : pExpr->space_def->fields[ > : pExpr->iColumn].name, target)); > : - > : - /* If the column has type NUMBER, it may currently be > : stored as an > : - * integer. Use OP_Realify to make sure it is really real. > : - */ > : - if (pExpr->iColumn >= 0 && def->fields[ > : - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { > : - sqlVdbeAddOp1(v, OP_Realify, target); > : - } > : break; > : } > : > : diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > : index 9e763ed85..0a3c904b1 100644 > : --- a/src/box/sql/vdbe.c > : +++ b/src/box/sql/vdbe.c > : @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ > : break; > : } > : > : -/* Opcode: Realify P1 * * * * > : - * > : - * If register P1 holds an integer convert it to a real value. > : - * > : - * This opcode is used when extracting information from a column that > : - * has float type. Such column values may still be stored as > : - * integers, for space efficiency, but after extraction we want them > : - * to have only a real value. > : - */ > : -case OP_Realify: { /* in1 */ > : - pIn1 = &aMem[pOp->p1]; > : - if (mem_is_int(pIn1)) { > : - mem_to_double(pIn1); > : - } > : - break; > : -} > : - > : /* Opcode: Cast P1 P2 * * * > : * Synopsis: type(r[P1]) > : * > : diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > : b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > : new file mode 100755 > : index 000000000..d29324a28 > : --- /dev/null > : +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua > : @@ -0,0 +1,39 @@ > : +#!/usr/bin/env tarantool > : +local test = require("sqltester") > : +test:plan(2) > : + > : +test:execsql([[ > : + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); > : + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); > : + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n > : = new.n; END; > : + INSERT INTO t1 VALUES (1, 1); > : + INSERT INTO t2 VALUES (1, 1); > : +]]) > : + > : +-- > : +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast > : in > : +-- field of type NUMBER. > : +-- > : +test:do_execsql_test( > : + "gh-5335-1", > : + [[ > : + SELECT i / 2, n / 2 FROM t1; > : + ]], { > : + 0, 0 > : + }) > : + > : +test:do_execsql_test( > : + "gh-5335-2", > : + [[ > : + SELECT i / 2, n / 2 FROM t2 GROUP BY n; > : + ]], { > : + 0, 0 > : + }) > : + > : +test:execsql([[ > : + DROP TRIGGER r; > : + DROP TABLE t1; > : + DROP TABLE t2; > : +]]) > : + > : +test:finish_test() > : -- > : 2.25.1 > >
This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify ...gh-5335-remove-wrong-double-to-int-cast.md | 4 ++ src/box/sql/expr.c | 13 ------- src/box/sql/vdbe.c | 17 -------- .../gh-5335-wrong-int-to-double-cast.test.lua | 39 +++++++++++++++++++ 4 files changed, 43 insertions(+), 30 deletions(-) create mode 100644 changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md new file mode 100644 index 000000000..b06805a7f --- /dev/null +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type + NUMBER (gh-5335). diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3772596d6..d2624516c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9e763ed85..0a3c904b1 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if (mem_is_int(pIn1)) { - mem_to_double(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua new file mode 100755 index 000000000..d29324a28 --- /dev/null +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -0,0 +1,39 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(2) + +test:execsql([[ + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; + INSERT INTO t1 VALUES (1, 1); + INSERT INTO t2 VALUES (1, 1); +]]) + +-- +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast in +-- field of type NUMBER. +-- +test:do_execsql_test( + "gh-5335-1", + [[ + SELECT i / 2, n / 2 FROM t1; + ]], { + 0, 0 + }) + +test:do_execsql_test( + "gh-5335-2", + [[ + SELECT i / 2, n / 2 FROM t2 GROUP BY n; + ]], { + 0, 0 + }) + +test:execsql([[ + DROP TRIGGER r; + DROP TABLE t1; + DROP TABLE t2; +]]) + +test:finish_test() -- 2.25.1
This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify ...gh-5335-remove-wrong-double-to-int-cast.md | 4 ++ src/box/sql/expr.c | 13 ------ src/box/sql/vdbe.c | 17 -------- .../gh-5335-wrong-int-to-double-cast.test.lua | 40 +++++++++++++++++++ 4 files changed, 44 insertions(+), 30 deletions(-) create mode 100644 changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md new file mode 100644 index 000000000..b06805a7f --- /dev/null +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type + NUMBER (gh-5335). diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3772596d6..d2624516c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9e763ed85..0a3c904b1 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if (mem_is_int(pIn1)) { - mem_to_double(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua new file mode 100755 index 000000000..76daa45e9 --- /dev/null +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -0,0 +1,40 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(2) + +test:execsql([[ + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); + -- This trigger is only needed to reproduce the error. + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; + INSERT INTO t1 VALUES (1, 1); + INSERT INTO t2 VALUES (1, 1); +]]) + +-- +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast in +-- field of type NUMBER. +-- +test:do_execsql_test( + "gh-5335-1", + [[ + SELECT i / 2, n / 2 FROM t1; + ]], { + 0, 0 + }) + +test:do_execsql_test( + "gh-5335-2", + [[ + SELECT i / 2, n / 2 FROM t2 GROUP BY n; + ]], { + 0, 0 + }) + +test:execsql([[ + DROP TRIGGER r; + DROP TABLE t1; + DROP TABLE t2; +]]) + +test:finish_test() -- 2.25.1
This opcode was used to convert INTEGER values to REAL. It is not necessary in Tarantool and causes errors. Due to OP_Realify two type of errors appeared: 1) In some cases in trigger INTEGER may be converted to DOUBLE. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") box.execute("INSERT INTO t VALUES (1, 1);") box.execute("SELECT i / 2, n / 2 FROM t;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0, 0.5] ... 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. For example: box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") box.execute("INSERT INTO t VALUES (1,1);") box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") Result: tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") --- - metadata: - name: COLUMN_1 type: number - name: COLUMN_2 type: number rows: - [0.5, 0.5] ... This patch removes OP_Realify, after which these errors disappear. Closes #5335 --- https://github.com/tarantool/tarantool/issues/5335 https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify ...gh-5335-remove-wrong-double-to-int-cast.md | 4 ++ src/box/sql/expr.c | 13 ------ src/box/sql/vdbe.c | 17 -------- .../gh-5335-wrong-int-to-double-cast.test.lua | 40 +++++++++++++++++++ 4 files changed, 44 insertions(+), 30 deletions(-) create mode 100644 changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md create mode 100755 test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua diff --git a/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md new file mode 100644 index 000000000..b06805a7f --- /dev/null +++ b/changelogs/unreleased/gh-5335-remove-wrong-double-to-int-cast.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Removed spontaneous conversion from INTEGER to DOUBLE in a field of type + NUMBER (gh-5335). diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3772596d6..d2624516c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3700,11 +3700,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp3(v, OP_Column, pAggInfo->sortingIdxPTab, pCol->iSorterColumn, target); - if (pCol->space_def->fields[pExpr->iAgg].type == - FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, - target); - } return target; } /* @@ -4260,14 +4255,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) (pExpr->iTable ? "new" : "old"), pExpr->space_def->fields[ pExpr->iColumn].name, target)); - - /* If the column has type NUMBER, it may currently be stored as an - * integer. Use OP_Realify to make sure it is really real. - */ - if (pExpr->iColumn >= 0 && def->fields[ - pExpr->iColumn].type == FIELD_TYPE_NUMBER) { - sqlVdbeAddOp1(v, OP_Realify, target); - } break; } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9e763ed85..0a3c904b1 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1445,23 +1445,6 @@ case OP_MustBeInt: { /* jump, in1 */ break; } -/* Opcode: Realify P1 * * * * - * - * If register P1 holds an integer convert it to a real value. - * - * This opcode is used when extracting information from a column that - * has float type. Such column values may still be stored as - * integers, for space efficiency, but after extraction we want them - * to have only a real value. - */ -case OP_Realify: { /* in1 */ - pIn1 = &aMem[pOp->p1]; - if (mem_is_int(pIn1)) { - mem_to_double(pIn1); - } - break; -} - /* Opcode: Cast P1 P2 * * * * Synopsis: type(r[P1]) * diff --git a/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua new file mode 100755 index 000000000..76daa45e9 --- /dev/null +++ b/test/sql-tap/gh-5335-wrong-int-to-double-cast.test.lua @@ -0,0 +1,40 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(2) + +test:execsql([[ + CREATE TABLE t1 (i NUMBER PRIMARY KEY, n NUMBER); + CREATE TABLE t2 (i NUMBER PRIMARY KEY, n NUMBER); + -- This trigger is only needed to reproduce the error. + CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN UPDATE t1 SET n = new.n; END; + INSERT INTO t1 VALUES (1, 1); + INSERT INTO t2 VALUES (1, 1); +]]) + +-- +-- Make sure that there are no unnecesary INTEGER to DOUBLE implicit cast in +-- field of type NUMBER. +-- +test:do_execsql_test( + "gh-5335-1", + [[ + SELECT i / 2, n / 2 FROM t1; + ]], { + 0, 0 + }) + +test:do_execsql_test( + "gh-5335-2", + [[ + SELECT i / 2, n / 2 FROM t2 GROUP BY n; + ]], { + 0, 0 + }) + +test:execsql([[ + DROP TRIGGER r; + DROP TABLE t1; + DROP TABLE t2; +]]) + +test:finish_test() -- 2.25.1
Hello,
On 04 авг 11:30, imeevma@tarantool.org wrote:
> This opcode was used to convert INTEGER values to REAL. It is not
> necessary in Tarantool and causes errors.
>
> Due to OP_Realify two type of errors appeared:
> 1) In some cases in trigger INTEGER may be converted to DOUBLE.
> For example:
> box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);")
> box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;")
> box.execute("INSERT INTO t VALUES (1, 1);")
> box.execute("SELECT i / 2, n / 2 FROM t;")
>
> Result:
> tarantool> box.execute("SELECT i / 2, n / 2 FROM t;")
> ---
> - metadata:
> - name: COLUMN_1
> type: number
> - name: COLUMN_2
> type: number
> rows:
> - [0, 0.5]
> ...
>
> 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER.
> For example:
> box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);")
> box.execute("INSERT INTO t VALUES (1,1);")
> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;")
>
> Result:
> tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;")
> ---
> - metadata:
> - name: COLUMN_1
> type: number
> - name: COLUMN_2
> type: number
> rows:
> - [0.5, 0.5]
> ...
>
> This patch removes OP_Realify, after which these errors disappear.
>
> Closes #5335
> ---
> https://github.com/tarantool/tarantool/issues/5335
> https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify
The patch LGTM.
--
Regards, Kirill Yukhin
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --] Hi team, QA LGTM -- Vitaliia Ioffe >Среда, 4 августа 2021, 17:08 +03:00 от Kirill Yukhin via Tarantool-patches <tarantool-patches@dev.tarantool.org>: > >Hello, > >On 04 авг 11:30, imeevma@tarantool.org wrote: >> This opcode was used to convert INTEGER values to REAL. It is not >> necessary in Tarantool and causes errors. >> >> Due to OP_Realify two type of errors appeared: >> 1) In some cases in trigger INTEGER may be converted to DOUBLE. >> For example: >> box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") >> box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;") >> box.execute("INSERT INTO t VALUES (1, 1);") >> box.execute("SELECT i / 2, n / 2 FROM t;") >> >> Result: >> tarantool> box.execute("SELECT i / 2, n / 2 FROM t;") >> --- >> - metadata: >> - name: COLUMN_1 >> type: number >> - name: COLUMN_2 >> type: number >> rows: >> - [0, 0.5] >> ... >> >> 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER. >> For example: >> box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);") >> box.execute("INSERT INTO t VALUES (1,1);") >> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") >> >> Result: >> tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;") >> --- >> - metadata: >> - name: COLUMN_1 >> type: number >> - name: COLUMN_2 >> type: number >> rows: >> - [0.5, 0.5] >> ... >> >> This patch removes OP_Realify, after which these errors disappear. >> >> Closes #5335 >> --- >> https://github.com/tarantool/tarantool/issues/5335 >> https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify >The patch LGTM. > >-- >Regards, Kirill Yukhin [-- Attachment #2: Type: text/html, Size: 2695 bytes --]
Hello,
On 04 авг 11:30, imeevma@tarantool.org wrote:
> This opcode was used to convert INTEGER values to REAL. It is not
> necessary in Tarantool and causes errors.
>
> Due to OP_Realify two type of errors appeared:
> 1) In some cases in trigger INTEGER may be converted to DOUBLE.
> For example:
> box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);")
> box.execute("CREATE TRIGGER t AFTER INSERT ON t FOR EACH ROW BEGIN UPDATE t SET n = new.n; END;")
> box.execute("INSERT INTO t VALUES (1, 1);")
> box.execute("SELECT i / 2, n / 2 FROM t;")
>
> Result:
> tarantool> box.execute("SELECT i / 2, n / 2 FROM t;")
> ---
> - metadata:
> - name: COLUMN_1
> type: number
> - name: COLUMN_2
> type: number
> rows:
> - [0, 0.5]
> ...
>
> 2) If SELECT uses GROUP BY then it may return DOUBLE instead of INTEGER.
> For example:
> box.execute("CREATE TABLE t (i NUMBER PRIMARY KEY, n NUMBER);")
> box.execute("INSERT INTO t VALUES (1,1);")
> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;")
>
> Result:
> tarantool> box.execute("SELECT i / 2, n / 2 FROM t GROUP BY n;")
> ---
> - metadata:
> - name: COLUMN_1
> type: number
> - name: COLUMN_2
> type: number
> rows:
> - [0.5, 0.5]
> ...
>
> This patch removes OP_Realify, after which these errors disappear.
>
> Closes #5335
> ---
> https://github.com/tarantool/tarantool/issues/5335
> https://github.com/tarantool/tarantool/tree/imeevma/gh-5335-remove-op-realify
I've checked your patch into 2.7, 2.8 and master.
--
Regards, Kirill Yukhin