[tarantool-patches] [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[tarantool-patches] [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[Roman Khabibov]

Added tests for removed SQL functions "CREATE TEMP TABLE" and "CREATE
TEMPORARY". There is exeptions, when these functions are used. Tests catch its.

Closes #2166
---
 test/sql-tap/table.test.lua | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 6e10561..4882b18 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(56)
+test:plan(58)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -611,6 +611,24 @@ test:do_execsql2_test(
 --     SELECT * FROM [t4"abc];
 --   }
 -- } {cnt 1 max(b+c) 5}
+test:do_catchsql_test(
+	"temp",
+	[[
+		CREATE TEMP TABLE
+	]], {
+	-- <temp>
+	1, "near \"TEMP\": syntax error"
+	-- <temp>
+	})
+test:do_catchsql_test(
+	"temporary",
+	[[
+		CREATE TEMPORARY TABLE
+	]], {
+	-- <temporary>
+	1, "near \"TEMPORARY\": syntax error"
+	-- <temporary>
+	})
 test:do_execsql2_test(
     "table-8.6",
     [[
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[Vladislav Shpilevoy]

Hello. My congratulations on your first contribution!

The patch is almost perfect with 2 minor comments:

1. Please, in the commit title write a Tarantool submodule prefix. For example,
your patch is about SQL, so you would write 'sql: ' prefix.

On 06/05/2018 01:34, Roman Khabibov wrote:
> Added tests for removed SQL functions "CREATE TEMP TABLE" and "CREATE
> TEMPORARY". There is exeptions, when these functions are used. Tests catch its.
> 
> Closes #2166
> ---
>   test/sql-tap/table.test.lua | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
> index 6e10561..4882b18 100755
> --- a/test/sql-tap/table.test.lua
> +++ b/test/sql-tap/table.test.lua
> @@ -1,6 +1,6 @@
>   #!/usr/bin/env tarantool
>   test = require("sqltester")
> -test:plan(56)
> +test:plan(58)
>   
>   --!./tcltestrunner.lua
>   -- 2001 September 15
> @@ -611,6 +611,24 @@ test:do_execsql2_test(
>   --     SELECT * FROM [t4"abc];
>   --   }
>   -- } {cnt 1 max(b+c) 5}

2. Before the test cases write in comments what do you test here, and
write a link to the issue in the format 'gh-NNNN: description'.
Example: https://github.com/tarantool/tarantool/blob/2.0/test/sql/iproto.test.lua#L122

> +test:do_catchsql_test(
> +	"temp",
> +	[[
> +		CREATE TEMP TABLE
> +	]], {
> +	-- <temp>
> +	1, "near \"TEMP\": syntax error"
> +	-- <temp>
> +	})
> +test:do_catchsql_test(
> +	"temporary",
> +	[[
> +		CREATE TEMPORARY TABLE
> +	]], {
> +	-- <temporary>
> +	1, "near \"TEMPORARY\": syntax error"
> +	-- <temporary>
> +	})
>   test:do_execsql2_test(
>       "table-8.6",
>       [[
> 

[tarantool-patches] Re: [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[roman.habibov1]

diff --git a/commits/1 b/commits/1
index 27ef2f6..dee20d6 100644

@@ -5,7 +5,7 @@ Date: Sat, 5 May 2018 01:45:06 +0300
 Subject: [PATCH v1 1/1] sql: tests for CREATE TEMP TABLE and  CREATE TEMPORARY
  TABLE

07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
> 1. Please, in the commit title write a Tarantool submodule prefix. For example,
> your patch is about SQL, so you would write 'sql: ' prefix.

diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 4882b18..21213bc 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -611,6 +611,9 @@ test:do_execsql2_test(
 --     SELECT * FROM [t4"abc];
 --   }
 -- } {cnt 1 max(b+c) 5}
+
+-- gh-2166 functions with TEMP and TEMPORARY are removed, tests needed to show that
+
 test:do_catchsql_test(
 	"temp",
 	[[

07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>2. Before the test cases write in comments what do you test here, and
>:write a link to the issue in the format 'gh-NNNN: description'.

[tarantool-patches] Re: [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[Vladislav Shpilevoy]

On 07/05/2018 21:50, roman.habibov1@yandex.ru wrote:
> diff --git a/commits/1 b/commits/1
> index 27ef2f6..dee20d6 100644
> 
> @@ -5,7 +5,7 @@ Date: Sat, 5 May 2018 01:45:06 +0300
>   Subject: [PATCH v1 1/1] sql: tests for CREATE TEMP TABLE and  CREATE TEMPORARY
>    TABLE
> 
> 07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>> 1. Please, in the commit title write a Tarantool submodule prefix. For example,
>> your patch is about SQL, so you would write 'sql: ' prefix.
> 
> diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
> index 4882b18..21213bc 100755
> --- a/test/sql-tap/table.test.lua
> +++ b/test/sql-tap/table.test.lua
> @@ -611,6 +611,9 @@ test:do_execsql2_test(
>   --     SELECT * FROM [t4"abc];
>   --   }
>   -- } {cnt 1 max(b+c) 5}
> +
> +-- gh-2166 functions with TEMP and TEMPORARY are removed, tests needed to show that

Please, do not write 'tests needed to show that' - all tests are needed to
show something, it is obvious.

At second, align the comments by 66 maximal symbols in line, and put a dot at
the end of sentence.

> +
>   test:do_catchsql_test(
>   	"temp",
>   	[[
> 
> 07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>> 2. Before the test cases write in comments what do you test here, and
>> :write a link to the issue in the format 'gh-NNNN: description'.
> 

[tarantool-patches] Re: [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[roman.habibov1]

diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 4882b18..3aa81f0 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -611,6 +611,9 @@ test:do_execsql2_test(
 --     SELECT * FROM [t4"abc];
 --   }
 -- } {cnt 1 max(b+c) 5}
+
+-- gh-2166 Functions with TEMP and TEMPORARY was removed before.
+
 test:do_catchsql_test(
 	"temp",
 	[[

08.05.2018, 02:12, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
> On 07/05/2018 21:50, roman.habibov1@yandex.ru wrote:
>>  diff --git a/commits/1 b/commits/1
>>  index 27ef2f6..dee20d6 100644
>>
>>  @@ -5,7 +5,7 @@ Date: Sat, 5 May 2018 01:45:06 +0300
>>    Subject: [PATCH v1 1/1] sql: tests for CREATE TEMP TABLE and CREATE TEMPORARY
>>     TABLE
>>
>>  07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>>>  1. Please, in the commit title write a Tarantool submodule prefix. For example,
>>>  your patch is about SQL, so you would write 'sql: ' prefix.
>>
>>  diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
>>  index 4882b18..21213bc 100755
>>  --- a/test/sql-tap/table.test.lua
>>  +++ b/test/sql-tap/table.test.lua
>>  @@ -611,6 +611,9 @@ test:do_execsql2_test(
>>    -- SELECT * FROM [t4"abc];
>>    -- }
>>    -- } {cnt 1 max(b+c) 5}
>>  +
>>  +-- gh-2166 functions with TEMP and TEMPORARY are removed, tests needed to show that
>
> Please, do not write 'tests needed to show that' - all tests are needed to
> show something, it is obvious.
>
> At second, align the comments by 66 maximal symbols in line, and put a dot at
> the end of sentence.
>
>>  +
>>    test:do_catchsql_test(
>>            "temp",
>>            [[
>>
>>  07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>>>  2. Before the test cases write in comments what do you test here, and
>>>  :write a link to the issue in the format 'gh-NNNN: description'.

[tarantool-patches] Re: [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[Vladislav Shpilevoy]

LGTM. Nikita, please, take a look.

On 08/05/2018 02:30, roman.habibov1@yandex.ru wrote:
> diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
> index 4882b18..3aa81f0 100755
> --- a/test/sql-tap/table.test.lua
> +++ b/test/sql-tap/table.test.lua
> @@ -611,6 +611,9 @@ test:do_execsql2_test(
>   --     SELECT * FROM [t4"abc];
>   --   }
>   -- } {cnt 1 max(b+c) 5}
> +
> +-- gh-2166 Functions with TEMP and TEMPORARY was removed before.
> +
>   test:do_catchsql_test(
>   	"temp",
>   	[[
> 
> 08.05.2018, 02:12, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>> On 07/05/2018 21:50, roman.habibov1@yandex.ru wrote:
>>>   diff --git a/commits/1 b/commits/1
>>>   index 27ef2f6..dee20d6 100644
>>>
>>>   @@ -5,7 +5,7 @@ Date: Sat, 5 May 2018 01:45:06 +0300
>>>     Subject: [PATCH v1 1/1] sql: tests for CREATE TEMP TABLE and CREATE TEMPORARY
>>>      TABLE
>>>
>>>   07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>>>>   1. Please, in the commit title write a Tarantool submodule prefix. For example,
>>>>   your patch is about SQL, so you would write 'sql: ' prefix.
>>>
>>>   diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
>>>   index 4882b18..21213bc 100755
>>>   --- a/test/sql-tap/table.test.lua
>>>   +++ b/test/sql-tap/table.test.lua
>>>   @@ -611,6 +611,9 @@ test:do_execsql2_test(
>>>     -- SELECT * FROM [t4"abc];
>>>     -- }
>>>     -- } {cnt 1 max(b+c) 5}
>>>   +
>>>   +-- gh-2166 functions with TEMP and TEMPORARY are removed, tests needed to show that
>>
>> Please, do not write 'tests needed to show that' - all tests are needed to
>> show something, it is obvious.
>>
>> At second, align the comments by 66 maximal symbols in line, and put a dot at
>> the end of sentence.
>>
>>>   +
>>>     test:do_catchsql_test(
>>>             "temp",
>>>             [[
>>>
>>>   07.05.2018, 16:08, "Vladislav Shpilevoy" <v.shpilevoy@tarantool.org>:
>>>>   2. Before the test cases write in comments what do you test here, and
>>>>   :write a link to the issue in the format 'gh-NNNN: description'.

[tarantool-patches] Re: [PATCH v1 1/1] Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[n.pettik]

 Hello, Roman.

Firstly, please specify subject of commit. In your particular case,
test definitely belongs to SQL part of project, so it is better to write
'sql: Tests for CREATE TEMP TABLE and  CREATE TEMPORARY TABLE'.
Notice, that commit subject should fit into 50 chars lenght.
Read this guide: https://tarantool.io/en/doc/2.0/dev_guide/developer_guidelines.html#how-to-write-a-commit-message

Secondly, after '---' delimiter (which separates commit message and
body of the patch) put link to your branch and corresponding issue.
Read this: https://tarantool.io/en/doc/2.0/dev_guide/developer_guidelines.html#how-to-submit-a-patch-for-review

Then, don't spam with commits. If you are willing to update your remote
branch with new version of the same code, use 'force' option:
git push -f. As for your patch, it must consist of only 1 commit.
Please, update your branch so than I can take a loot at whole patch.

>+-- gh-2166 Functions with TEMP and TEMPORARY was removed before.

Typo: ‘were’. And not functions, but tables themselves, I guess.

[tarantool-patches] Re: [PATCH v1 1/1] sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[n.pettik]

Please, read guidelines carefully.

>sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

According to our docs:
— 2. Try to limit the subject line to 50 characters or so.
— 3. Start the subject line with a capital letter unless it prefixed with a subsystem name and semicolon…

Also, it is not clear what have you done with these ’tests’: removed/fixed/added etc.

>+
>+-- gh-2166 Tables with TEMP and TEMPORARY were removed before.
>+       

Leading space at the last line. Enable showing whitespaces to avoid such codestyle violations.

>+       })
>+test:do_catchsql_test(
>+       "temporary",

>+       1, "near \"TEMPORARY\": syntax error"
>+       -- <temporary>
>+       })
>test:do_execsql2_test(

Separate tests with blank line.

I would also add tests (or just rewrite yours) which check that complete CREATE TABLE statement doesn’t support TEMP clause:

CREATE TEMP TABLE t1(…);

CREATE TEMPORARY TABLE t1(…);

[tarantool-patches] Re: [PATCH v1 1/1] sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[roman.habibov1]

> Please, read guidelines carefully.
>
>> sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE
>
> According to our docs:
> — 2. Try to limit the subject line to 50 characters or so.
> — 3. Start the subject line with a capital letter unless it prefixed with a subsystem name and semicolon…
Sorry. I fixed that.

> Also, it is not clear what have you done with these ’tests’: removed/fixed/added etc.
That too.

>> +
>> +-- gh-2166 Tables with TEMP and TEMPORARY were removed before.
>> +
>
> Leading space at the last line. Enable showing whitespaces to avoid such codestyle violations.
>> + })
>> +test:do_catchsql_test(
>> + "temporary",
>
>> + 1, "near \"TEMPORARY\": syntax error"
>> + -- <temporary>
>> + })
>> test:do_execsql2_test(
That too.

> Separate tests with blank line.
diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 70b22d2..84eb884 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -623,6 +623,7 @@ test:do_catchsql_test(
 	1, "near \"TEMP\": syntax error"
 	-- <temp>
 	})
+
 test:do_catchsql_test(
 	"temporary",
 	[[
@@ -632,6 +633,7 @@ test:do_catchsql_test(
 	1, "near \"TEMPORARY\": syntax error"
 	-- <temporary>
 	})
+
 test:do_execsql2_test(
     "table-8.6",
     [[

> I would also add tests (or just rewrite yours) which check that complete CREATE TABLE statement doesn’t support TEMP clause:
>
> CREATE TEMP TABLE t1(…);
>
> CREATE TEMPORARY TABLE t1(…);
diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index c71e4b9..70b22d2 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -613,11 +613,11 @@ test:do_execsql2_test(
 -- } {cnt 1 max(b+c) 5}
 
 -- gh-2166 Tables with TEMP and TEMPORARY were removed before.
-	
+
 test:do_catchsql_test(
 	"temp",
 	[[
-		CREATE TEMP TABLE
+		CREATE TEMP TABLE t1();
 	]], {
 	-- <temp>
 	1, "near \"TEMP\": syntax error"
@@ -626,7 +626,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
 	"temporary",
 	[[
-		CREATE TEMPORARY TABLE
+		CREATE TEMPORARY TABLE t1();
 	]], {
 	-- <temporary>
 	1, "near \"TEMPORARY\": syntax error"

[tarantool-patches] Re: [PATCH v1 1/1] sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE

[n.pettik]

Don’t hurry when preparing patch or making fixes.
Remember: ‘haste makes waste’.

>> Please, read guidelines carefully.
>> 
>>> sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE
>> 
>> According to our docs:
>> — 2. Try to limit the subject line to 50 characters or so.
>> — 3. Start the subject line with a capital letter unless it prefixed with a subsystem name and semicolon…
> Sorry. I fixed that.

No, you didn’t. Read it again. I see old violations on your branch.

>There is exeptions, when these clauses are used.

Typo: ’there are’. Use some spell-checker or whatever.
Rephrase whole sentence: it seems to be misleading.

>> I would also add tests (or just rewrite yours) which check that complete CREATE TABLE statement doesn’t support TEMP clause:
>> 
>> CREATE TEMP TABLE t1(…);
>> 
>> CREATE TEMPORARY TABLE t1(…);
> +		CREATE TEMP TABLE t1();
> +		CREATE TEMPORARY TABLE t1();


Cmon, use complete form of CREATE TABLE statement.
You can’t create table with no columns.

[tarantool-patches] Re: [PATCH v1 1/1] sql: added tests for TEMP and TEMPORARY clauses

[roman.habibov1]

>>>  Please, read guidelines carefully.
>>>
>>>>  sql: Tests for CREATE TEMP TABLE and CREATE TEMPORARY TABLE
>>>
>>>  According to our docs:
>>>  — 2. Try to limit the subject line to 50 characters or so.
>>>  — 3. Start the subject line with a capital letter unless it prefixed with a subsystem name and semicolon…
>>  Sorry. I fixed that.
>
> No, you didn’t. Read it again. I see old violations on your branch.
But now?

>> There is exeptions, when these clauses are used.
>
> Typo: ’there are’. Use some spell-checker or whatever.
> Rephrase whole sentence: it seems to be misleading.
Rephrased.

>>>  I would also add tests (or just rewrite yours) which check that complete CREATE TABLE statement doesn’t support TEMP clause:
>>>
>>>  CREATE TEMP TABLE t1(…);
>>>
>>>  CREATE TEMPORARY TABLE t1(…);
>>  + CREATE TEMP TABLE t1();
>>  + CREATE TEMPORARY TABLE t1();
>
> Cmon, use complete form of CREATE TABLE statement.
> You can’t create table with no columns.
diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 84eb884..f0a1cbc 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -617,7 +617,7 @@ test:do_execsql2_test(
 test:do_catchsql_test(
 	"temp",
 	[[
-		CREATE TEMP TABLE t1();
+		CREATE TEMP TABLE t1(col1 int, col2 int);
 	]], {
 	-- <temp>
 	1, "near \"TEMP\": syntax error"
@@ -627,7 +627,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
 	"temporary",
 	[[
-		CREATE TEMPORARY TABLE t1();
+		CREATE TEMPORARY TABLE t1(col1 int, col2 int);
 	]], {
 	-- <temporary>
 	1, "near \"TEMPORARY\": syntax error"

[tarantool-patches] Re: [PATCH v1 1/1] sql: added tests for TEMP and TEMPORARY clauses

[n.pettik]

> +		CREATE TEMP TABLE t1(col1 int, col2 int);
> +		CREATE TEMPORARY TABLE t1(col1 int, col2 int);

You can’t create table this way: you must specify primary key.

[tarantool-patches] Re: [PATCH v1 1/1] sql: added tests for TEMP and TEMPORARY clauses

[roman.habibov1]

14.05.2018, 17:50, "n.pettik" <korablev@tarantool.org>:
>>  + CREATE TEMP TABLE t1(col1 int, col2 int);
>>  + CREATE TEMPORARY TABLE t1(col1 int, col2 int);
>
> You can’t create table this way: you must specify primary key.
diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index f0a1cbc..b0e99b0 100755
--- a/test/sql-tap/table.test.lua
+++ b/test/sql-tap/table.test.lua
@@ -617,7 +617,7 @@ test:do_execsql2_test(
 test:do_catchsql_test(
 	"temp",
 	[[
-		CREATE TEMP TABLE t1(col1 int, col2 int);
+		CREATE TEMP TABLE t1(a INTEGER PRIMARY KEY, b VARCHAR(10));
 	]], {
 	-- <temp>
 	1, "near \"TEMP\": syntax error"
@@ -627,7 +627,7 @@ test:do_catchsql_test(
 test:do_catchsql_test(
 	"temporary",
 	[[
-		CREATE TEMPORARY TABLE t1(col1 int, col2 int);
+		CREATE TEMPORARY TABLE t1(a INTEGER PRIMARY KEY, b VARCHAR(10));
 	]], {
 	-- <temporary>
 	1, "near \"TEMPORARY\": syntax error"

[tarantool-patches] Re: [PATCH v1 1/1] sql: added tests for TEMP and TEMPORARY clauses

[n.pettik]

Thanks, now patch looks OK to me.

[tarantool-patches] Re: [PATCH v1 1/1] sql: added tests for TEMP and TEMPORARY clauses

[Kirill Yukhin]

Hi guys,
On 19 мая 03:28, n.pettik wrote:
> Thanks, now patch looks OK to me.

Thanks for your contribution. I've checked in the
patch into 2.0 branch.

--
Regards, Kirill Yukhin