From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [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 8BBA0430407 for ; Fri, 21 Aug 2020 15:34:11 +0300 (MSK) Date: Fri, 21 Aug 2020 15:34:09 +0300 From: Mergen Imeev Message-ID: <20200821123409.GA219863@tarantool.org> References: <56f685097fb1120e36bf03114a32d567e668a2a2.1597998754.git.imeevma@gmail.com> <20200821092130.GC6452@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200821092130.GC6452@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: remove implicit cast in bitwise operations List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the review. My answer and new patch below. On Fri, Aug 21, 2020 at 09:21:30AM +0000, Nikita Pettik wrote: > On 21 Aug 11:40, imeevma@tarantool.org wrote: > > This patch removes the implicit conversion from STRING to INTEGER from > > bitwise operations. However, DOUBLE can still be implicitly converted to > > INTEGER. > > I see no test involving doubles in bitwise operations. Does it make > any sense at all? Even if it doesn't, it is legal to implicitly convert DOUBLE to INTEGER. However, when I tried to add tests for this case, I found an error in my patch. I re-made patch. Now in these opcodes we convert MEM to INTEGER, which I tried to avoid in previous patch. I did this to fix a bug where result of the operation is DOUBLE if one of the operands is DOUBLE. It didn't help, the result still has DOUBLE type. I decided to left conversion since it looks right here because all operands must be INTEGERS. This wouldn't work for arithmetic operations though. New patch: >From 3515ada4b363062cf9caa5d550ea40770e8a5e65 Mon Sep 17 00:00:00 2001 From: Mergen Imeev Date: Tue, 18 Aug 2020 18:18:59 +0300 Subject: [PATCH] sql: remove implicit cast in bitwise operations This patch removes the implicit conversion from STRING to INTEGER from bitwise operations. However, DOUBLE can still be implicitly converted to INTEGER. Follow-up #3809 diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index c0143a6b1..42228c435 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1989,23 +1989,26 @@ case OP_ShiftRight: { /* same as TK_RSHIFT, in1, in2, out3 */ pIn1 = &aMem[pOp->p1]; pIn2 = &aMem[pOp->p2]; + enum mp_type type1 = mem_mp_type(pIn1); + enum mp_type type2 = mem_mp_type(pIn2); pOut = vdbe_prepare_null_out(p, pOp->p3); - if ((pIn1->flags | pIn2->flags) & MEM_Null) { + if (type1 == MP_NIL || type2 == MP_NIL) { /* Force NULL be of type INTEGER. */ pOut->field_type = FIELD_TYPE_INTEGER; break; } - bool unused; - if (sqlVdbeIntValue(pIn2, (int64_t *) &iA, &unused) != 0) { + if (mem_convert_to_integer(pIn2) != 0) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, sql_value_to_diag_str(pIn2), "integer"); goto abort_due_to_error; } - if (sqlVdbeIntValue(pIn1, (int64_t *) &iB, &unused) != 0) { + if (mem_convert_to_integer(pIn1) != 0) { diag_set(ClientError, ER_SQL_TYPE_MISMATCH, sql_value_to_diag_str(pIn1), "integer"); goto abort_due_to_error; } + iA = pIn2->u.i; + iB = pIn1->u.i; op = pOp->opcode; if (op==OP_BitAnd) { iA &= iB; @@ -2621,19 +2624,19 @@ case OP_Not: { /* same as TK_NOT, in1, out2 */ */ case OP_BitNot: { /* same as TK_BITNOT, in1, out2 */ pIn1 = &aMem[pOp->p1]; + enum mp_type type = mem_mp_type(pIn1); pOut = vdbe_prepare_null_out(p, pOp->p2); /* Force NULL be of type INTEGER. */ pOut->field_type = FIELD_TYPE_INTEGER; - if ((pIn1->flags & MEM_Null)==0) { - int64_t i; - bool is_neg; - if (sqlVdbeIntValue(pIn1, &i, &is_neg) != 0) { - diag_set(ClientError, ER_SQL_TYPE_MISMATCH, - sql_value_to_diag_str(pIn1), "integer"); - goto abort_due_to_error; - } - mem_set_i64(pOut, ~i); + if (type == MP_NIL) { + break; + } + if (mem_convert_to_integer(pIn1) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + sql_value_to_diag_str(pIn1), "integer"); + goto abort_due_to_error; } + mem_set_i64(pOut, ~pIn1->u.i); break; } diff --git a/test/sql/types.result b/test/sql/types.result index caedbf409..70247471e 100644 --- a/test/sql/types.result +++ b/test/sql/types.result @@ -2846,3 +2846,93 @@ box.execute([[SELECT 1 - '2';]]) - null - 'Type mismatch: can not convert 2 to numeric' ... +-- +-- Make sure there is no implicit string-to-number conversion in bitwise +-- operations. +-- +box.execute([[SELECT '1' | 2;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT '1' & 2;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT '1' << 2;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT '1' >> 2;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT ~'1';]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT 1 | '2';]]) +--- +- null +- 'Type mismatch: can not convert 2 to integer' +... +box.execute([[SELECT 1 & '2';]]) +--- +- null +- 'Type mismatch: can not convert 2 to integer' +... +box.execute([[SELECT 1 << '2';]]) +--- +- null +- 'Type mismatch: can not convert 2 to integer' +... +box.execute([[SELECT 1 >> '2';]]) +--- +- null +- 'Type mismatch: can not convert 2 to integer' +... +-- Make sure that DOUBLE implicitly cast to INTEGER in bitwise operations. +box.execute([[SELECT 3.5 | 1.3;]]) +--- +- metadata: + - name: COLUMN_1 + type: double + rows: + - [3] +... +box.execute([[SELECT 3.5 & 1.3;]]) +--- +- metadata: + - name: COLUMN_1 + type: double + rows: + - [1] +... +box.execute([[SELECT 3.5 << 1.3;]]) +--- +- metadata: + - name: COLUMN_1 + type: double + rows: + - [6] +... +box.execute([[SELECT 3.5 >> 1.3;]]) +--- +- metadata: + - name: COLUMN_1 + type: double + rows: + - [1] +... +box.execute([[SELECT ~3.5;]]) +--- +- metadata: + - name: COLUMN_1 + type: double + rows: + - [-4] +... diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua index 844a6b670..c000f4d13 100644 --- a/test/sql/types.test.lua +++ b/test/sql/types.test.lua @@ -638,3 +638,24 @@ box.execute([[SELECT 1 % '2';]]) box.execute([[SELECT 1 * '2';]]) box.execute([[SELECT 1 / '2';]]) box.execute([[SELECT 1 - '2';]]) + +-- +-- Make sure there is no implicit string-to-number conversion in bitwise +-- operations. +-- +box.execute([[SELECT '1' | 2;]]) +box.execute([[SELECT '1' & 2;]]) +box.execute([[SELECT '1' << 2;]]) +box.execute([[SELECT '1' >> 2;]]) +box.execute([[SELECT ~'1';]]) +box.execute([[SELECT 1 | '2';]]) +box.execute([[SELECT 1 & '2';]]) +box.execute([[SELECT 1 << '2';]]) +box.execute([[SELECT 1 >> '2';]]) + +-- Make sure that DOUBLE implicitly cast to INTEGER in bitwise operations. +box.execute([[SELECT 3.5 | 1.3;]]) +box.execute([[SELECT 3.5 & 1.3;]]) +box.execute([[SELECT 3.5 << 1.3;]]) +box.execute([[SELECT 3.5 >> 1.3;]]) +box.execute([[SELECT ~3.5;]])