[Tarantool-patches] [PATCH v1 2/2] sql: remove implicit cast in bitwise operations

Mergen Imeev imeevma at tarantool.org
Tue Sep 29 16:00:46 MSK 2020


Привет! Спасибо за ревью. Мои ответы ниже.

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 at 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 <imeevma at gmail.com>
> > > > 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 <imeevma at gmail.com>
> > 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';]])


More information about the Tarantool-patches mailing list