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

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

[Kirill Yukhin]

Since currently query planner is unable to use HASH indexes
and attempt to use it will likely lead to SEGFAULT, this
patch raises an error on attempt to open VDBE cursor
against HASH index.

Closes #4659
---

ChangeLog entry:
## Bugs fixed
### SQL
* Block using HASH indexes in SQL since scheduler 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

 src/box/sql/build.c                            |  6 +++++
 test/sql-tap/gh-4659-block-hash-index.test.lua | 35 ++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
 create mode 100755 test/sql-tap/gh-4659-block-hash-index.test.lua

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");
+		parse_context->is_aborted = true;
+		return -1;
+	}
 	return sqlVdbeAddOp4(parse_context->pVdbe, OP_IteratorOpen, cursor,
 				 index_id, 0, (void *) space, P4_SPACEPTR);
 }
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
--- /dev/null
+++ b/test/sql-tap/gh-4659-block-hash-index.test.lua
@@ -0,0 +1,35 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(2)
+
+-- gh-4782 - Raise an error in case space features HASH index.
+-- Make sure that in case of attempt to use HASH index
+-- from within SQL statement - an error is raised.
+-- This is actually a stub until we are unable to employ
+-- HASH index while planning a query.
+
+f = {
+    {'1', 'unsigned'},
+    {'2', 'string'},
+}
+
+s = box.schema.create_space("T1", {format = f})
+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"
+    })
+
+s = box.schema.create_space("T2", {format = f})
+s:create_index('PK', {parts = {'2'}})
+s:create_index('SK', {type = 'hash', parts = {'1'}})
+
+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"
+    })
+
+test:finish_test()
-- 
1.8.3.1

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

[Nikita Pettik]

On 16 Jun 18:06, Kirill Yukhin wrote:
> Since currently query planner is unable to use HASH indexes
> and attempt to use it will likely lead to SEGFAULT, this
> patch raises an error on attempt to open VDBE cursor
> against HASH index.
> 
> Closes #4659
> ---
> 
> ChangeLog entry:
> ## Bugs fixed
> ### SQL
> * Block using HASH indexes in SQL since scheduler

scheduler -> planner.

> is unable  to use it properly (gh-4659).

Not only hash indexes, but any other except for tree indexes.
 
> 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

>  src/box/sql/build.c                            |  6 +++++
>  test/sql-tap/gh-4659-block-hash-index.test.lua | 35 ++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>  create mode 100755 test/sql-tap/gh-4659-block-hash-index.test.lua
> 
> 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."

> 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
> --- /dev/null
> +++ b/test/sql-tap/gh-4659-block-hash-index.test.lua
> @@ -0,0 +1,35 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(2)
> +
> +-- gh-4782 - Raise an error in case space features HASH index.
> +-- Make sure that in case of attempt to use HASH index
> +-- from within SQL statement - an error is raised.
> +-- This is actually a stub until we are unable to employ
> +-- HASH index while planning a query.
> +
> +f = {
> +    {'1', 'unsigned'},
> +    {'2', 'string'},
> +}
> +
> +s = box.schema.create_space("T1", {format = f})
> +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"
> +    })
> +
> +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.

> +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"
> +    })
> +
> +test:finish_test()
> -- 
> 1.8.3.1
> 

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

[Kirill Yukhin]

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()

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

[Nikita Pettik]

On 17 Jun 17:45, Kirill Yukhin wrote:
> Hello,
> 
> Thanks a lot for review.
> 
> My answers inlined. Iterative patch in the bottom.
> Branch force-pushed.
>

LGTM

The only thing missing is a docbot request (in case it is still
undocumented).

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

[Kirill Yukhin]

Hello,

On 16 июн 18:06, Kirill Yukhin wrote:
> Since currently query planner is unable to use HASH indexes
> and attempt to use it will likely lead to SEGFAULT, this
> patch raises an error on attempt to open VDBE cursor
> against HASH index.
> 
> Closes #4659

I've checked the patch into 2.3, 2.4 and master.

--
Regards, Kirill Yukhin