From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 59656452566 for ; Fri, 8 Nov 2019 17:09:01 +0300 (MSK) Date: Fri, 8 Nov 2019 17:09:00 +0300 From: Nikita Pettik Message-ID: <20191108140900.GB82024@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: make NUMBER to be union of SQL numeric types List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org On 07 Nov 15:13, imeevma@tarantool.org wrote: > This patch makes number to be union of UNSIGNED, INTEGER and > DOUBLE numeric types. The first two types represented by Tarantool > UNSIGNED and INTEGER types. Nit: integer's range includes unsigned's one. > Currently there is not DOUBLE type in Tarantool. TBO from commit message it's not clear what does you patch change. Please, elaborate it. What is more, your patch modifies user-visible behaviour, so I think doc-bot request is required. > Closes #4233 > Closes #4463 Please, split patch into two ones: each fixes corresponding problem. > --- > https://github.com/tarantool/tarantool/issues/4233 > https://github.com/tarantool/tarantool/tree/imeevma/gh-4233-fix-number-field-type-in-sql > > src/box/sql/vdbe.c | 15 +---- > src/box/sql/vdbeInt.h | 1 - > src/box/sql/vdbemem.c | 57 ++----------------- > test/sql-tap/cast.test.lua | 32 +++++------ > test/sql-tap/e_select1.test.lua | 2 +- > test/sql-tap/numcast.test.lua | 104 ++++++++++++++++++++++++++++++++++- > test/sql-tap/select3.test.lua | 2 +- > test/sql-tap/sort.test.lua | 12 ++-- > test/sql-tap/tkt-91e2e8ba6f.test.lua | 12 ++-- > test/sql/integer-overflow.result | 2 +- > test/sql/types.result | 2 +- > 11 files changed, 140 insertions(+), 101 deletions(-) What I really dislike about this patch is the fact that it eliminates ability to convert value to floating point representation. As far as I remember you told that you was working on introducing floating point field type in NoSQL Tarantool. And after making floating point type available in SQL part of code you deleted should be resurrected. I suggest firstly introduce floating point type in NoSQL, then make it available in SQL and finally rename CAST(source TO NUMBER) -> source TO FLOAT > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 03c63d8..2ceee1a 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1586,7 +1586,6 @@ case OP_Subtract: /* same as TK_MINUS, in1, in2, out3 */ > case OP_Multiply: /* same as TK_STAR, in1, in2, out3 */ > case OP_Divide: /* same as TK_SLASH, in1, in2, out3 */ > case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ > - char bIntint; /* Started out as two integer operands */ > u32 flags; /* Combined MEM_* flags from both inputs */ > u16 type1; /* Numeric type of left operand */ > u16 type2; /* Numeric type of right operand */ > @@ -1609,7 +1608,6 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ > bool is_lhs_neg = pIn1->flags & MEM_Int; > bool is_rhs_neg = pIn2->flags & MEM_Int; > bool is_res_neg; > - bIntint = 1; > switch( pOp->opcode) { > case OP_Add: { > if (sql_add_int(iA, is_lhs_neg, iB, is_rhs_neg, > @@ -1649,7 +1647,6 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ > } > mem_set_int(pOut, iB, is_res_neg); > } else { > - bIntint = 0; > if (sqlVdbeRealValue(pIn1, &rA) != 0) { > diag_set(ClientError, ER_SQL_TYPE_MISMATCH, > sql_value_to_diag_str(pIn1), "numeric"); > @@ -1660,6 +1657,7 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ > sql_value_to_diag_str(pIn2), "numeric"); > goto abort_due_to_error; > } > + assert(((type1 | type2) & MEM_Real) != 0); > switch( pOp->opcode) { > case OP_Add: rB += rA; break; > case OP_Subtract: rB -= rA; break; > @@ -1685,9 +1683,6 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */ > } > pOut->u.r = rB; > MemSetTypeFlag(pOut, MEM_Real); > - if (((type1|type2)&MEM_Real)==0 && !bIntint) { > - mem_apply_integer_type(pOut); > - } > } Seems like this change of OP_Remainder is barely related to patch..