Re: [Tarantool-patches] [PATCH] sql: raise an error on attempt to use HASH index in SQL

[Kirill Yukhin <kyukhin@tarantool.org>]

Hello,

Thanks a lot for review.

My answers inlined. Iterative patch in the bottom.
Branch force-pushed.

On 16 июн 23:08, Nikita Pettik wrote:
> On 16 Jun 18:06, Kirill Yukhin wrote:
> > ChangeLog entry:
> > ## Bugs fixed
> > ### SQL
> > * Block using HASH indexes in SQL since scheduler
> 
> scheduler -> planner.
> Not only hash indexes, but any other except for tree indexes.  

Updated.
ChangeLog entry:
## Bugs fixed
### SQL
* Block using any type of indexes except for TREE in SQL, since
query planner is unable  to use it properly (gh-4659).
 
> > Issue: https://github.com/tarantool/tarantool/issues/4659
> > Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-4659-block-hash-index
> 
> Tests fail on your branch. See https://travis-ci.org/github/tarantool/tarantool/jobs/698493716

Yeah, I forgot to add the hunk to final patch. Fixed.
Testing pass https://gitlab.com/tarantool/tarantool/-/pipelines/157247263

> > diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> > index 0b60d2e..b429a4e 100644
> > --- a/src/box/sql/build.c
> > +++ b/src/box/sql/build.c
> > @@ -722,6 +722,12 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
> >  		      struct space *space)
> >  {
> >  	assert(space != NULL);
> > +	if (space->index[index_id]->def->type != TREE) {
> > +		diag_set(ClientError, ER_UNSUPPORTED, "SQL",
> > +			 "using of non-TREE index type");
> 
> Nit: I guess 'of' is reundant in this sentence. Also I'd include
> hint in error message like: "... type. Please, use INDEXED BY clause
> to force using proper index."

Added.
 
> > diff --git a/test/sql-tap/gh-4659-block-hash-index.test.lua b/test/sql-tap/gh-4659-block-hash-index.test.lua
> > new file mode 100755
> > index 0000000..8c84e2d
> > +s = box.schema.create_space("T2", {format = f})
> > +s:create_index('PK', {parts = {'2'}})
> > +s:create_index('SK', {type = 'hash', parts = {'1'}})
> 
> Add a couple of tests covering other index types.

Added.

--
Regards, Kirill Yukhin

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index b429a4e..714fbfb 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -722,9 +722,12 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
 		      struct space *space)
 {
 	assert(space != NULL);
-	if (space->index[index_id]->def->type != TREE) {
+	struct index *idx = index_find(space, index_id);
+	assert(idx != NULL);
+	if (idx->def->type != TREE) {
 		diag_set(ClientError, ER_UNSUPPORTED, "SQL",
-			 "using of non-TREE index type");
+			 "using non-TREE index type. Please, use " \
+			 "INDEXED BY clause to force using proper index.");
 		parse_context->is_aborted = true;
 		return -1;
 	}
diff --git a/test/sql-tap/gh-4659-block-hash-index.test.lua b/test/sql-tap/gh-4659-block-hash-index.test.lua
index 8c84e2d..f0f3cb9 100755
--- a/test/sql-tap/gh-4659-block-hash-index.test.lua
+++ b/test/sql-tap/gh-4659-block-hash-index.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(2)
+test:plan(4)
 
 -- gh-4782 - Raise an error in case space features HASH index.
 -- Make sure that in case of attempt to use HASH index
@@ -11,6 +11,7 @@ test:plan(2)
 f = {
     {'1', 'unsigned'},
     {'2', 'string'},
+    {'3', 'array'}
 }
 
 s = box.schema.create_space("T1", {format = f})
@@ -19,17 +20,31 @@ s:create_index('PK', {type = 'hash', parts = {'1'}})
 test:do_catchsql_test(
     "blocked-hash-index-1",
     "SELECT * FROM T1", {
-        1, "SQL does not support using of non-TREE index type"
+        1, "SQL does not support using non-TREE index type. Please, use INDEXED BY clause to force using proper index."
     })
 
 s = box.schema.create_space("T2", {format = f})
 s:create_index('PK', {parts = {'2'}})
-s:create_index('SK', {type = 'hash', parts = {'1'}})
+s:create_index('SK1', {type = 'hash', parts = {'1'}})
+s:create_index('SK2', {type = 'bitset', parts = {'1'}})
+s:create_index('SK3', {type = 'rtree', parts = {'3'}})
 
 test:do_catchsql_test(
-    "blocked-hash-index-1",
-    "SELECT * FROM T2 INDEXED BY SK", {
-        1, "SQL does not support using of non-TREE index type"
+    "blocked-hash-index-2",
+    "SELECT * FROM T2 INDEXED BY SK1", {
+        1, "SQL does not support using non-TREE index type. Please, use INDEXED BY clause to force using proper index."
+    })
+
+test:do_catchsql_test(
+    "blocked-hash-index-3",
+    "SELECT * FROM T2 INDEXED BY SK2", {
+        1, "SQL does not support using non-TREE index type. Please, use INDEXED BY clause to force using proper index."
+    })
+
+test:do_catchsql_test(
+    "blocked-hash-index-4",
+    "SELECT * FROM T2 INDEXED BY SK3", {
+        1, "SQL does not support using non-TREE index type. Please, use INDEXED BY clause to force using proper index."
     })
 
 test:finish_test()