[tarantool-patches] [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[tarantool-patches] [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/issues/4422
Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option

See particular commits for details.

Nikita Pettik (2):
  Do not log error in engine_find()
  sql: allow to specify engine in CREATE TABLE stmt

 extra/mkkeywordhash.c    |  1 +
 src/box/engine.h         |  4 +--
 src/box/sql/parse.y      | 27 +++++++++++++--
 test/sql/engine.result   | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/engine.test.lua | 30 +++++++++++++++++
 5 files changed, 141 insertions(+), 6 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/2] Do not log error in engine_find()

[Nikita Pettik]

Error logging in engine_find() seems to be redundant: error message is
displayed twice (since its callers always push error on the top).  Let's
remove this duplication.
---
 src/box/engine.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/box/engine.h b/src/box/engine.h
index a302b3bca..ef694da00 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -235,10 +235,8 @@ static inline struct engine *
 engine_find(const char *name)
 {
 	struct engine *engine = engine_by_name(name);
-	if (engine == NULL) {
+	if (engine == NULL)
 		diag_set(ClientError, ER_NO_SUCH_ENGINE, name);
-		diag_log();
-	}
 	return engine;
 }
 
-- 
2.15.1

[tarantool-patches] [PATCH 2/2] sql: allow to specify engine in CREATE TABLE stmt

[Nikita Pettik]

Closes #4422

@TarantoolBot document
Title: Introduce <WITH ENGINE> clause for CREATE TABLE statement

To allow user to specify engine as per table option, CREATE TABLE
statement has been extended with optional <WITH ENGINE = engine_name>
clause. This clause comes at the end of CREATE TABLE statement.
For instance:

CREATE TABLE t_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'vinyl';

Name of engine is considered to be string literal ergo should be
enclosed in single quotation marks and be lower-cased. Note that engine
specified in WITH ENGINE clause overwrites default engine, which is set
via 'pragma sql_default_engine'.
---
 extra/mkkeywordhash.c    |  1 +
 src/box/sql/parse.y      | 27 +++++++++++++--
 test/sql/engine.result   | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/engine.test.lua | 30 +++++++++++++++++
 4 files changed, 140 insertions(+), 3 deletions(-)

diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index a6d0f79a4..d0aa39a43 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -111,6 +111,7 @@ static Keyword aKeywordTable[] = {
   { "DISTINCT",               "TK_DISTINCT",    ALWAYS,           true  },
   { "DROP",                   "TK_DROP",        ALWAYS,           true  },
   { "END",                    "TK_END",         ALWAYS,           true  },
+  { "ENGINE",                 "TK_ENGINE",      ALWAYS,           true  },
   { "EACH",                   "TK_EACH",        TRIGGER,          true  },
   { "ELSE",                   "TK_ELSE",        ALWAYS,           true  },
   { "ESCAPE",                 "TK_ESCAPE",      ALWAYS,           true  },
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ca6df0a40..be3c5c399 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -177,7 +177,7 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). {
 
 ///////////////////// The CREATE TABLE statement ////////////////////////////
 //
-cmd ::= create_table create_table_args.
+cmd ::= create_table create_table_args with_opts create_table_end.
 create_table ::= createkw TABLE ifnotexists(E) nm(Y). {
   create_table_def_init(&pParse->create_table_def, &Y, E);
   pParse->create_table_def.new_space = sqlStartTable(pParse, &Y);
@@ -189,10 +189,31 @@ createkw(A) ::= CREATE(A).  {disableLookaside(pParse);}
 ifnotexists(A) ::= .              {A = 0;}
 ifnotexists(A) ::= IF NOT EXISTS. {A = 1;}
 
-create_table_args ::= LP columnlist RP. {
-  sqlEndTable(pParse);
+create_table_args ::= LP columnlist RP.
+
+with_opts ::= WITH engine_opts.
+with_opts ::= .
+
+engine_opts ::= ENGINE EQ STRING(A). {
+  /* Note that specifying engine clause overwrites default engine. */
+  if (A.n > ENGINE_NAME_MAX) {
+    diag_set(ClientError, ER_CREATE_SPACE,
+             pParse->create_table_def.new_space->def->name,
+             "space engine name is too long");
+    pParse->is_aborted = true;
+    return;
+  }
+  /* Need to dequote name. */
+  char *normalized_name = sql_name_from_token(pParse->db, &A);
+  if (normalized_name == NULL)
+    return;
+  memcpy(pParse->create_table_def.new_space->def->engine_name, normalized_name,
+         strlen(normalized_name) + 1);
+  sqlDbFree(pParse->db, normalized_name);
 }
 
+create_table_end ::= . { sqlEndTable(pParse); }
+
 /*
  * CREATE TABLE AS SELECT is broken. To be re-implemented
  * in gh-3223.
diff --git a/test/sql/engine.result b/test/sql/engine.result
index cee9f7f32..3ee93ad19 100644
--- a/test/sql/engine.result
+++ b/test/sql/engine.result
@@ -48,3 +48,88 @@ box.execute("DROP TABLE t3_memtx;")
 ---
 - row_count: 1
 ...
+-- gh-4422: allow to specify engine in CREATE TABLE statement.
+--
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'vinyl'")
+---
+- row_count: 1
+...
+assert(box.space.T1_VINYL.engine == 'vinyl')
+---
+- true
+...
+box.execute("CREATE TABLE t1_memtx (id INT PRIMARY KEY) WITH ENGINE = 'memtx'")
+---
+- row_count: 1
+...
+assert(box.space.T1_MEMTX.engine == 'memtx')
+---
+- true
+...
+box.execute("pragma sql_default_engine='vinyl'")
+---
+- row_count: 0
+...
+box.execute("CREATE TABLE t2_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'vinyl'")
+---
+- row_count: 1
+...
+assert(box.space.T2_VINYL.engine == 'vinyl')
+---
+- true
+...
+box.execute("CREATE TABLE t2_memtx (id INT PRIMARY KEY) WITH ENGINE = 'memtx'")
+---
+- row_count: 1
+...
+assert(box.space.T2_MEMTX.engine == 'memtx')
+---
+- true
+...
+box.space.T1_VINYL:drop()
+---
+...
+box.space.T1_MEMTX:drop()
+---
+...
+box.space.T2_VINYL:drop()
+---
+...
+box.space.T2_MEMTX:drop()
+---
+...
+-- Name of engine considered to be string literal, so should be
+-- lowercased and quoted.
+--
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = VINYL")
+---
+- null
+- Syntax error near 'VINYL'
+...
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = vinyl")
+---
+- null
+- Syntax error near 'vinyl'
+...
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'VINYL'")
+---
+- null
+- Space engine 'VINYL' does not exist
+...
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = \"vinyl\"")
+---
+- null
+- Syntax error near '"vinyl"'
+...
+-- Make sure that wrong engine name is handled properly.
+--
+box.execute("CREATE TABLE t_wrong_engine (id INT PRIMARY KEY) WITH ENGINE = 'abc'")
+---
+- null
+- Space engine 'abc' does not exist
+...
+box.execute("CREATE TABLE t_long_engine_name (id INT PRIMARY KEY) WITH ENGINE = 'very_long_engine_name'")
+---
+- null
+- 'Failed to create space ''T_LONG_ENGINE_NAME'': space engine name is too long'
+...
diff --git a/test/sql/engine.test.lua b/test/sql/engine.test.lua
index 51d6939e7..112d3d3ff 100644
--- a/test/sql/engine.test.lua
+++ b/test/sql/engine.test.lua
@@ -15,3 +15,33 @@ assert(box.space.T3_MEMTX.engine == 'memtx')
 box.execute("DROP TABLE t1_vinyl;")
 box.execute("DROP TABLE t2_vinyl;")
 box.execute("DROP TABLE t3_memtx;")
+
+-- gh-4422: allow to specify engine in CREATE TABLE statement.
+--
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'vinyl'")
+assert(box.space.T1_VINYL.engine == 'vinyl')
+box.execute("CREATE TABLE t1_memtx (id INT PRIMARY KEY) WITH ENGINE = 'memtx'")
+assert(box.space.T1_MEMTX.engine == 'memtx')
+box.execute("pragma sql_default_engine='vinyl'")
+box.execute("CREATE TABLE t2_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'vinyl'")
+assert(box.space.T2_VINYL.engine == 'vinyl')
+box.execute("CREATE TABLE t2_memtx (id INT PRIMARY KEY) WITH ENGINE = 'memtx'")
+assert(box.space.T2_MEMTX.engine == 'memtx')
+
+box.space.T1_VINYL:drop()
+box.space.T1_MEMTX:drop()
+box.space.T2_VINYL:drop()
+box.space.T2_MEMTX:drop()
+
+-- Name of engine considered to be string literal, so should be
+-- lowercased and quoted.
+--
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = VINYL")
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = vinyl")
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'VINYL'")
+box.execute("CREATE TABLE t1_vinyl (id INT PRIMARY KEY) WITH ENGINE = \"vinyl\"")
+
+-- Make sure that wrong engine name is handled properly.
+--
+box.execute("CREATE TABLE t_wrong_engine (id INT PRIMARY KEY) WITH ENGINE = 'abc'")
+box.execute("CREATE TABLE t_long_engine_name (id INT PRIMARY KEY) WITH ENGINE = 'very_long_engine_name'")
-- 
2.15.1

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[Vladislav Shpilevoy]

Thanks for the patch! LGTM.

On 19/08/2019 16:49, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/issues/4422
> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option
> 
> See particular commits for details.
> 
> Nikita Pettik (2):
>   Do not log error in engine_find()
>   sql: allow to specify engine in CREATE TABLE stmt
> 
>  extra/mkkeywordhash.c    |  1 +
>  src/box/engine.h         |  4 +--
>  src/box/sql/parse.y      | 27 +++++++++++++--
>  test/sql/engine.result   | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>  test/sql/engine.test.lua | 30 +++++++++++++++++
>  5 files changed, 141 insertions(+), 6 deletions(-)
> 

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/19 17:54]:
> Branch: https://github.com/tarantool/tarantool/issues/4422
> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option
> 

I am utterly disappointed with the fact that instead of fixing a
broken feature that is already there and have been waiting years
for a fix you spend time on working on a new feature
which *no one* except your highly aesthetic self has asked for :(

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 2/2] sql: allow to specify engine in CREATE TABLE stmt

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/19 17:54]:
> @TarantoolBot document
> Title: Introduce <WITH ENGINE> clause for CREATE TABLE statement
> 
> To allow user to specify engine as per table option, CREATE TABLE
> statement has been extended with optional <WITH ENGINE = engine_name>
> clause. This clause comes at the end of CREATE TABLE statement.
> For instance:
> 
> CREATE TABLE t_vinyl (id INT PRIMARY KEY) WITH ENGINE = 'vinyl';
> 
> Name of engine is considered to be string literal ergo should be
> enclosed in single quotation marks and be lower-cased. Note that engine
> specified in WITH ENGINE clause overwrites default engine, which is set
> via 'pragma sql_default_engine'.

the wording should not contain word 'with'. 

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[n.pettik]

> On 20 Aug 2019, at 14:57, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * Nikita Pettik <korablev@tarantool.org> [19/08/19 17:54]:
>> Branch: https://github.com/tarantool/tarantool/issues/4422
>> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option
>> 
> 
> I am utterly disappointed with the fact that instead of fixing a
> broken feature that is already there and have been waiting years
> for a fix

What feature exactly are you talking about?

> you spend time on working on a new feature

It was done in one day, not so much time spent I guess.

> which *no one* except your highly aesthetic self has asked for :(

Every week I see questions in public chat like “how to create vinyl table
using SQL facilities” and that’s a fact.

> -- 
> Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/08/20 15:15]:
> 
> 
> > On 20 Aug 2019, at 14:57, Konstantin Osipov <kostja@tarantool.org> wrote:
> > 
> > * Nikita Pettik <korablev@tarantool.org> [19/08/19 17:54]:
> >> Branch: https://github.com/tarantool/tarantool/issues/4422
> >> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option
> >> 
> > 
> > I am utterly disappointed with the fact that instead of fixing a
> > broken feature that is already there and have been waiting years
> > for a fix
> 
> What feature exactly are you talking about?

The one we discussed and agreed on, set engine='vinyl'

> > you spend time on working on a new feature
> 
> It was done in one day, not so much time spent I guess.
> 
> > which *no one* except your highly aesthetic self has asked for :(
> 
> Every week I see questions in public chat like “how to create vinyl table
> using SQL facilities” and that’s a fact.

The answer should be: use set engine='vinyl'.

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[n.pettik]

> On 20 Aug 2019, at 16:49, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * n.pettik <korablev@tarantool.org> [19/08/20 15:15]:
>> 
>> 
>>> On 20 Aug 2019, at 14:57, Konstantin Osipov <kostja@tarantool.org> wrote:
>>> 
>>> * Nikita Pettik <korablev@tarantool.org> [19/08/19 17:54]:
>>>> Branch: https://github.com/tarantool/tarantool/issues/4422
>>>> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option
>>>> 
>>> 
>>> I am utterly disappointed with the fact that instead of fixing a
>>> broken feature that is already there and have been waiting years
>>> for a fix
>> 
>> What feature exactly are you talking about?
> 
> The one we discussed and agreed on, set engine=‘vinyl'

It’s about default engine, not per table option.

Speaking of ‘waiting years for a fix’ - there’s no even corresponding issue
except for one which you created last week.

>>> you spend time on working on a new feature
>> 
>> It was done in one day, not so much time spent I guess.
>> 
>>> which *no one* except your highly aesthetic self has asked for :(
>> 
>> Every week I see questions in public chat like “how to create vinyl table
>> using SQL facilities” and that’s a fact.
> 
> The answer should be: use set engine='vinyl'.
> 
> -- 
> Konstantin Osipov, Moscow, Russia
> 

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/08/20 16:58]:
> >> What feature exactly are you talking about?
> > 
> > The one we discussed and agreed on, set engine=‘vinyl'
> 
> It’s about default engine, not per table option.

Has anyone asked for per-table option?


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/2] Introduce WITH ENGINE option for CREATE TABLE stmt

[Kirill Yukhin]

Hello,

On 19 Aug 17:49, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/issues/4422
> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4422-with-engine-option
> 
> See particular commits for details.
> 
> Nikita Pettik (2):
>   Do not log error in engine_find()
>   sql: allow to specify engine in CREATE TABLE stmt

I've checked your patches into master.

--
Regards, Kirill Yukhin