From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B573F6EC55; Fri, 30 Jul 2021 10:18:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B573F6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627629496; bh=RED3vLVDiBl/GpDHymd4n+Sip/2XL5GecL4JgGDG/YA=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=FuSTQ3MVNooAa457SaMcd3taInbmmqwv24LlWJrdmwhOmHfCHPi1NTUgTHBnSr9Wt i1VrCOHMiQFJPD3l9/ghLua5hz7NHi9zTDVmySAcyv2db3Al2ARJTkiI1fNiRX/BJC vI8rX7wFSwD7WSvXOtVNHYlob/U9DUKxJ6KBroDU= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2FB056EC55 for ; Fri, 30 Jul 2021 10:18:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2FB056EC55 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1m9Mmb-0001Cr-Ox; Fri, 30 Jul 2021 10:18:14 +0300 Date: Fri, 30 Jul 2021 10:18:12 +0300 To: Timur Safin Message-ID: <20210730071812.GA7937@tarantool.org> References: <005f01d78512$8defd690$a9cf83b0$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <005f01d78512$8defd690$a9cf83b0$@tarantool.org> X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD941C43E597735A9C30A5AB0699C09BB51E5FD76225F0C99C3182A05F538085040BDFFCBFEBD63B4D2F795DE6F7B586B72EFC84F10697D71D04CEB51588C4357FC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE761966F250AC1AE21EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063725D748B084CAA27D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88816F9F4291D6CD1CD462B0D75B75186117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66F6A3E018CF4DC80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A55F7ECBAABE22DBC937760119D3655DB9CAC590C4B0059823D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344B4608D01B1CB59A039DF3BD949E40610F19B3FE84610FA8065DD78CBBADEFE7C27F095DE8EF21F11D7E09C32AA3244C88A70DEBC77D438A22DFA511815CBC16F26BFA4C8A6946B8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojWBddABnKmoLSSC6aBRWdag== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D5BA0D9D4F10B5B3B13AB5103C712EE9283D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove OP_Realify X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > : 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 > >