From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 701664429E1 for ; Wed, 17 Jun 2020 17:45:37 +0300 (MSK) Date: Wed, 17 Jun 2020 17:45:36 +0300 From: Kirill Yukhin Message-ID: <20200617144536.yit7qljhf26efm5e@tarantool.org> References: <3002c51d7ae67c00320046838922aa9edd0b4d90.1592319653.git.kyukhin@tarantool.org> <20200616230812.GA9358@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200616230812.GA9358@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: raise an error on attempt to use HASH index in SQL List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.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()