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 00E5E469719 for ; Sun, 27 Sep 2020 12:11:54 +0300 (MSK) Date: Sun, 27 Sep 2020 12:11:52 +0300 From: Mergen Imeev Message-ID: <20200927091152.GA60576@tarantool.org> References: <56f685097fb1120e36bf03114a32d567e668a2a2.1597998754.git.imeevma@gmail.com> <20200821092130.GC6452@tarantool.org> <20200821123409.GA219863@tarantool.org> <20200917133648.GF10599@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200917133648.GF10599@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 Привет! Спасибо за ревью. Мои ответы и новый патч ниже. On Thu, Sep 17, 2020 at 01:36:48PM +0000, Nikita Pettik wrote: > On 21 Aug 15:34, Mergen Imeev wrote: > > 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. > > Не понял ничего..Как эта штука должна работать?? > box.execute([[SELECT 3.5 | 1.3;]]) > metadata: > name: COLUMN_1 > type: double > rows: > [3] -- WTF > В соответствии с правилами implicit cast between numeric values. В любом случае, убрал возможность делать значения типа DOUBLE операндами битовых операций. > Давай по-честному выдавать ошибку, когда в битовые операции суем не инты. Сделал. > > > 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 > > Новый патч: >From a630c39d486769d1a684e56b15bbf5107e7bef66 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. Only integer values allowed in bitwise operations. Follow-up #3809 diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index c0143a6b1..5d1ab8c94 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 (type2 != MP_INT && type2 != MP_UINT) { 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 (type1 != MP_INT && type1 != MP_UINT) { 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,18 @@ 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 (type != MP_INT && type != MP_UINT) { + 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..a79818b30 100644 --- a/test/sql/types.result +++ b/test/sql/types.result @@ -2846,3 +2846,284 @@ 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 only values of type INTEGER can be operands of bitwise +-- operations. +-- +box.execute([[SELECT 3 | 1;]]) +--- +- metadata: + - name: COLUMN_1 + type: integer + rows: + - [3] +... +box.execute([[SELECT 3 | 1.1;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT 3 | '1';]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT 3 | true;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT 3 | X'31';]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 1 | 3;]]) +--- +- metadata: + - name: COLUMN_1 + type: integer + rows: + - [3] +... +box.execute([[SELECT 1.1 | 3;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT '1' | 3;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT true | 3;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT X'31' | 3;]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 3 & 1;]]) +--- +- metadata: + - name: COLUMN_1 + type: integer + rows: + - [1] +... +box.execute([[SELECT 3 & 1.1;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT 3 & '1';]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT 3 & true;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT 3 & X'31';]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 1.1 & 3;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT '1' & 3;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT true & 3;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT X'31' & 3;]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 3 << 1;]]) +--- +- metadata: + - name: COLUMN_1 + type: integer + rows: + - [6] +... +box.execute([[SELECT 3 << 1.1;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT 3 << '1';]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT 3 << true;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT 3 << X'31';]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 1.1 << 3;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT '1' << 3;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT true << 3;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT X'31' << 3;]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 3 >> 1;]]) +--- +- metadata: + - name: COLUMN_1 + type: integer + rows: + - [1] +... +box.execute([[SELECT 3 >> 1.1;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT 3 >> '1';]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT 3 >> true;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT 3 >> X'31';]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT 1.1 >> 3;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT '1' >> 3;]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT true >> 3;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT X'31' >> 3;]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... +box.execute([[SELECT ~1;]]) +--- +- metadata: + - name: COLUMN_1 + type: integer + rows: + - [-2] +... +box.execute([[SELECT ~1.1;]]) +--- +- null +- 'Type mismatch: can not convert 1.1 to integer' +... +box.execute([[SELECT ~'1';]]) +--- +- null +- 'Type mismatch: can not convert 1 to integer' +... +box.execute([[SELECT ~true;]]) +--- +- null +- 'Type mismatch: can not convert TRUE to integer' +... +box.execute([[SELECT ~X'31';]]) +--- +- null +- 'Type mismatch: can not convert varbinary to integer' +... diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua index 844a6b670..bb922e78f 100644 --- a/test/sql/types.test.lua +++ b/test/sql/types.test.lua @@ -638,3 +638,68 @@ 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 only values of type INTEGER can be operands of bitwise +-- operations. +-- +box.execute([[SELECT 3 | 1;]]) +box.execute([[SELECT 3 | 1.1;]]) +box.execute([[SELECT 3 | '1';]]) +box.execute([[SELECT 3 | true;]]) +box.execute([[SELECT 3 | X'31';]]) +box.execute([[SELECT 1 | 3;]]) +box.execute([[SELECT 1.1 | 3;]]) +box.execute([[SELECT '1' | 3;]]) +box.execute([[SELECT true | 3;]]) +box.execute([[SELECT X'31' | 3;]]) + +box.execute([[SELECT 3 & 1;]]) +box.execute([[SELECT 3 & 1.1;]]) +box.execute([[SELECT 3 & '1';]]) +box.execute([[SELECT 3 & true;]]) +box.execute([[SELECT 3 & X'31';]]) +box.execute([[SELECT 1.1 & 3;]]) +box.execute([[SELECT '1' & 3;]]) +box.execute([[SELECT true & 3;]]) +box.execute([[SELECT X'31' & 3;]]) + +box.execute([[SELECT 3 << 1;]]) +box.execute([[SELECT 3 << 1.1;]]) +box.execute([[SELECT 3 << '1';]]) +box.execute([[SELECT 3 << true;]]) +box.execute([[SELECT 3 << X'31';]]) +box.execute([[SELECT 1.1 << 3;]]) +box.execute([[SELECT '1' << 3;]]) +box.execute([[SELECT true << 3;]]) +box.execute([[SELECT X'31' << 3;]]) + +box.execute([[SELECT 3 >> 1;]]) +box.execute([[SELECT 3 >> 1.1;]]) +box.execute([[SELECT 3 >> '1';]]) +box.execute([[SELECT 3 >> true;]]) +box.execute([[SELECT 3 >> X'31';]]) +box.execute([[SELECT 1.1 >> 3;]]) +box.execute([[SELECT '1' >> 3;]]) +box.execute([[SELECT true >> 3;]]) +box.execute([[SELECT X'31' >> 3;]]) + +box.execute([[SELECT ~1;]]) +box.execute([[SELECT ~1.1;]]) +box.execute([[SELECT ~'1';]]) +box.execute([[SELECT ~true;]]) +box.execute([[SELECT ~X'31';]])