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 CD730469719 for ; Tue, 29 Sep 2020 16:00:48 +0300 (MSK) Date: Tue, 29 Sep 2020 16:00:46 +0300 From: Mergen Imeev Message-ID: <20200929130046.GA20600@tarantool.org> References: <56f685097fb1120e36bf03114a32d567e668a2a2.1597998754.git.imeevma@gmail.com> <20200821092130.GC6452@tarantool.org> <20200821123409.GA219863@tarantool.org> <20200917133648.GF10599@tarantool.org> <20200927091152.GA60576@tarantool.org> <20200928171340.GC14909@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200928171340.GC14909@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 Mon, Sep 28, 2020 at 05:13:40PM +0000, Nikita Pettik wrote: > On 27 Sep 12:11, Mergen Imeev wrote: > > Привет! Спасибо за ревью. Мои ответы и новый патч ниже. > > > > 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; > > Это будет работать с интами > 2^32? В любом случае, я думаю, надо добавить > тестов.. > С 2^32 проблем не должно бить, но возможны проблемы с числами равными или больше 2^63. Тесты предлагаю делать в рамках тикета по этой теме (#5364). > select 18446744073709551615 | 4 > --- > - metadata: > - name: COLUMN_1 > type: integer > rows: > - [-1] > ... > > Выглядит неверно... > Создал на это отдельный тикет(#5364), предлагаю решать этот вопрос там. > > 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';]])