Re: [Tarantool-patches] [PATCH v5 3/6] sql: change comparison between numbers using index

[Mergen Imeev <imeevma@tarantool.org>]

Thank you for review! My answers and diff below.

On Fri, Sep 18, 2020 at 08:08:21AM +0000, Nikita Pettik wrote:
> On 21 Aug 12:19, imeevma@tarantool.org wrote:
> > This patch disables number conversions in ApplyType in wherecode.c. This
> > allows conversions between numbers introduced in previous commit to be
> > used.
> > 
> > Part of #4230
> > ---
> >  src/box/sql/sqlInt.h                 |  2 +
> >  src/box/sql/vdbe.c                   |  3 +-
> >  src/box/sql/wherecode.c              | 76 +---------------------------
> >  test/sql-tap/in4.test.lua            |  4 +-
> >  test/sql-tap/join.test.lua           |  4 +-
> >  test/sql-tap/tkt-9a8b09f8e6.test.lua |  8 +--
> >  test/sql/types.result                | 54 ++++++++++++++++++++
> >  test/sql/types.test.lua              | 12 +++++
> >  8 files changed, 79 insertions(+), 84 deletions(-)
> > 
> > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> > index adf90d824..1e6f0f41f 100644
> > --- a/src/box/sql/sqlInt.h
> > +++ b/src/box/sql/sqlInt.h
> > @@ -2309,6 +2309,8 @@ struct Parse {
> >  #define OPFLAG_SYSTEMSP      0x20	/* OP_Open**: set if space pointer
> >  					 * points to system space.
> >  					 */
> > +/** OP_ApplyType: Do not convert numbers. */
> 
> Useless comment.
I do not think so, since it shows that this MACRO should be used only in
OP_ApplyType.

> 
> > +#define OPFLAG_DO_NOT_CONVERT_NUMBERS	0x01
> > diff --git a/test/sql/types.result b/test/sql/types.result
> > index 442245186..8810a9f82 100644
> > --- a/test/sql/types.result
> > +++ b/test/sql/types.result
> > @@ -2795,3 +2795,57 @@ box.execute([[DROP TABLE ts;]])
> >  ---
> >  - row_count: 1
> >  ...
> > +--
> > +-- gh-4230: Make sure the comparison between numbers that use
> > +-- index is working correctly.
> > +--
> > +box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, a DOUBLE);]])
> > +---
> > +- row_count: 1
> > +...
> > +box.execute([[INSERT INTO t VALUES (1, ?);]], {2^60})
> 
> I'd place also eqp statements to make sure that index is really used.
Done. It helped to find that there wasn't an index in column 'a'. Also fixed.

> 
> > +---
> > +- row_count: 1
> > +...



Diff:


diff --git a/test/sql/types.result b/test/sql/types.result
index 8810a9f82..9a7578d75 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -2799,7 +2796,7 @@ box.execute([[DROP TABLE ts;]])
 -- gh-4230: Make sure the comparison between numbers that use
 -- index is working correctly.
 --
-box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, a DOUBLE);]])
+box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, a DOUBLE UNIQUE);]])
 ---
 - row_count: 1
 ...
@@ -2807,6 +2804,20 @@ box.execute([[INSERT INTO t VALUES (1, ?);]], {2^60})
 ---
 - row_count: 1
 ...
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE i > ?]], {2^70})
+---
+- metadata:
+  - name: selectid
+    type: integer
+  - name: order
+    type: integer
+  - name: from
+    type: integer
+  - name: detail
+    type: text
+  rows:
+  - [0, 0, 0, 'SEARCH TABLE T USING PRIMARY KEY (I>?) (~262144 rows)']
+...
 box.execute([[SELECT * FROM t WHERE i > ?]], {2^70})
 ---
 - metadata:
@@ -2816,6 +2827,20 @@ box.execute([[SELECT * FROM t WHERE i > ?]], {2^70})
     type: double
   rows: []
 ...
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE i > ?]], {-2^70})
+---
+- metadata:
+  - name: selectid
+    type: integer
+  - name: order
+    type: integer
+  - name: from
+    type: integer
+  - name: detail
+    type: text
+  rows:
+  - [0, 0, 0, 'SEARCH TABLE T USING PRIMARY KEY (I>?) (~262144 rows)']
+...
 box.execute([[SELECT * FROM t WHERE i > ?]], {-2^70})
 ---
 - metadata:
@@ -2826,6 +2851,20 @@ box.execute([[SELECT * FROM t WHERE i > ?]], {-2^70})
   rows:
   - [1, 1152921504606846976]
 ...
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE a = ?]], {2ULL^60ULL - 1ULL})
+---
+- metadata:
+  - name: selectid
+    type: integer
+  - name: order
+    type: integer
+  - name: from
+    type: integer
+  - name: detail
+    type: text
+  rows:
+  - [0, 0, 0, 'SEARCH TABLE T USING COVERING INDEX unique_unnamed_T_2 (A=?) (~1 row)']
+...
 box.execute([[SELECT * FROM t WHERE a = ?]], {2ULL^60ULL - 1ULL})
 ---
 - metadata:
@@ -2835,6 +2874,21 @@ box.execute([[SELECT * FROM t WHERE a = ?]], {2ULL^60ULL - 1ULL})
     type: double
   rows: []
 ...
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE a > ?]], {2ULL^60ULL - 1ULL})
+---
+- metadata:
+  - name: selectid
+    type: integer
+  - name: order
+    type: integer
+  - name: from
+    type: integer
+  - name: detail
+    type: text
+  rows:
+  - [0, 0, 0, 'SEARCH TABLE T USING COVERING INDEX unique_unnamed_T_2 (A>?) (~262144
+      rows)']
+...
 box.execute([[SELECT * FROM t WHERE a > ?]], {2ULL^60ULL - 1ULL})
 ---
 - metadata:
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index a23b12801..ae19167bd 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -628,10 +628,14 @@ box.execute([[DROP TABLE ts;]])
 -- gh-4230: Make sure the comparison between numbers that use
 -- index is working correctly.
 --
-box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, a DOUBLE);]])
+box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, a DOUBLE UNIQUE);]])
 box.execute([[INSERT INTO t VALUES (1, ?);]], {2^60})
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE i > ?]], {2^70})
 box.execute([[SELECT * FROM t WHERE i > ?]], {2^70})
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE i > ?]], {-2^70})
 box.execute([[SELECT * FROM t WHERE i > ?]], {-2^70})
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE a = ?]], {2ULL^60ULL - 1ULL})
 box.execute([[SELECT * FROM t WHERE a = ?]], {2ULL^60ULL - 1ULL})
+box.execute([[EXPLAIN QUERY PLAN SELECT * FROM t WHERE a > ?]], {2ULL^60ULL - 1ULL})
 box.execute([[SELECT * FROM t WHERE a > ?]], {2ULL^60ULL - 1ULL})
 box.execute([[DROP TABLE t;]])