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

[Mergen Imeev <imeevma@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 <imeevma@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

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;]])