From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 D9463469719 for ; Mon, 28 Sep 2020 20:13:41 +0300 (MSK) Date: Mon, 28 Sep 2020 17:13:40 +0000 From: Nikita Pettik Message-ID: <20200928171340.GC14909@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> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200927091152.GA60576@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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org 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? В любом случае, я думаю, надо добавить тестов.. select 18446744073709551615 | 4 --- - metadata: - name: COLUMN_1 type: integer rows: - [-1] ... Выглядит неверно... > 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';]])