[tarantool-patches] Re: [PATCH] sql: constraints definition among columns in CREATE TABLE()

[roman.habibov1@yandex.ru]

Hi! Thanks for review.

> Hi! Thanks for the patch! Please, fix one minor comment
> below.
> Please, add a test that I can not write multiple 'CONSTRAINT <name>'
> in a row.
>
>>        -- </table-15.1>
>>    })
Done.

commit 5bcc130bcabaa1f8efe8b12af35be5f2a4a05e6e
Author: Roman Khabibov <roman.habibov1@yandex.ru>
Date:   Sun Nov 18 15:54:53 2018 +0300

    sql: constraints def among columns in CREATE TABLE()
    
    Allow constraints to appear along with columns definitions. Disallow typing
    a constraint name without specifying the constraint and after.
    
    Closes #3504

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index abaa73736..5b9553017 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -176,15 +176,17 @@ createkw(A) ::= CREATE(A).  {disableLookaside(pParse);}
 ifnotexists(A) ::= .              {A = 0;}
 ifnotexists(A) ::= IF NOT EXISTS. {A = 1;}
 
-create_table_args ::= LP columnlist conslist_opt RP(E). {
+create_table_args ::= LP columnlist RP(E). {
   sqlite3EndTable(pParse,&E,0);
 }
 create_table_args ::= AS select(S). {
   sqlite3EndTable(pParse,0,S);
   sql_select_delete(pParse->db, S);
 }
+columnlist ::= columnlist COMMA tconsdef.
 columnlist ::= columnlist COMMA columnname carglist.
 columnlist ::= columnname carglist.
+columnlist ::= tconsdef.
 columnname(A) ::= nm(A) typedef(Y). {sqlite3AddColumn(pParse,&A,&Y);}
 
 // An IDENTIFIER can be a generic identifier, or one of several
@@ -232,9 +234,11 @@ nm(A) ::= id(A). {
 // "carglist" is a list of additional constraints that come after the
 // column name and column type in a CREATE TABLE statement.
 //
-carglist ::= carglist ccons.
+carglist ::= carglist cconsdef.
 carglist ::= .
-ccons ::= CONSTRAINT nm(X).           {pParse->constraintName = X;}
+cconsdef ::= cconsname ccons.
+cconsname ::= CONSTRAINT nm(X).           {pParse->constraintName = X;}
+cconsname ::= .                           {pParse->constraintName.n = 0;}
 ccons ::= DEFAULT term(X).            {sqlite3AddDefaultValue(pParse,&X);}
 ccons ::= DEFAULT LP expr(X) RP.      {sqlite3AddDefaultValue(pParse,&X);}
 ccons ::= DEFAULT PLUS term(X).       {sqlite3AddDefaultValue(pParse,&X);}
@@ -303,13 +307,9 @@ init_deferred_pred_opt(A) ::= .                       {A = 0;}
 init_deferred_pred_opt(A) ::= INITIALLY DEFERRED.     {A = 1;}
 init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE.    {A = 0;}
 
-conslist_opt ::= .
-conslist_opt ::= COMMA conslist.
-conslist ::= conslist tconscomma tcons.
-conslist ::= tcons.
-tconscomma ::= COMMA.            {pParse->constraintName.n = 0;}
-tconscomma ::= .
-tcons ::= CONSTRAINT nm(X).      {pParse->constraintName = X;}
+tconsdef ::= tconsname tcons.
+tconsname ::= CONSTRAINT nm(X).      {pParse->constraintName = X;}
+tconsname ::= .                      {pParse->constraintName.n = 0;}
 tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP.
                                  {sqlite3AddPrimaryKey(pParse,X,I,0);}
 tcons ::= UNIQUE LP sortlist(X) RP.
diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
index 039e2291e..ebdbc5b13 100755
--- a/test/sql-tap/check.test.lua
+++ b/test/sql-tap/check.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(60)
+test:plan(58)
 
 --!./tcltestrunner.lua
 -- 2005 November 2
@@ -270,59 +270,31 @@ test:do_catchsql_test(
         -- </check-2.6>
     })
 
--- Undocumented behavior:  The CONSTRAINT name clause can follow a constraint.
--- Such a clause is ignored.  But the parser must accept it for backwards
--- compatibility.
---
-test:do_execsql_test(
+-- gh-3504: Check the CONSTRAINT name clause can't follow a constraint.
+
+test:do_catchsql_test(
     "check-2.10",
     [[
         CREATE TABLE t2b(
-          x INTEGER CHECK( typeof(coalesce(x,0))=='integer' ) CONSTRAINT one,
-          y TEXT PRIMARY KEY constraint two,
-          z INTEGER,
-          UNIQUE(x,z) constraint three
+          x INTEGER CHECK( typeof(coalesce(x,0))=='integer' ) CONSTRAINT one
         );
     ]], {
         -- <check-2.10>
-
+        1, "near \")\": syntax error"
         -- </check-2.10>
     })
 
 test:do_catchsql_test(
     "check-2.11",
-    [[
-        INSERT INTO t2b VALUES('xyzzy','hi',5);
-    ]], {
-        -- <check-2.11>
-        1, "CHECK constraint failed: T2B"
-        -- </check-2.11>
-    })
-
-test:do_execsql_test(
-    "check-2.12",
     [[
         CREATE TABLE t2c(
-          x INTEGER CONSTRAINT x_one CONSTRAINT x_two primary key
-              CHECK( typeof(coalesce(x,0))=='integer' )
-              CONSTRAINT x_two CONSTRAINT x_three,
-          y INTEGER, z INTEGER,
-          CONSTRAINT u_one UNIQUE(x,y,z) CONSTRAINT u_two
+          x INTEGER CONSTRAINT one CHECK( typeof(coalesce(x,0))=='integer' )
+        CONSTRAINT two
         );
     ]], {
-        -- <check-2.12>
-
-        -- </check-2.12>
-    })
-
-test:do_catchsql_test(
-    "check-2.13",
-    [[
-        INSERT INTO t2c VALUES('xyzzy',7,8);
-    ]], {
-        -- <check-2.13>
-        1, "CHECK constraint failed: X_TWO"
-        -- </check-2.13>
+        -- <check-2.10>
+        1, "near \")\": syntax error"
+        -- </check-2.10>
     })
 
 test:do_execsql_test(
diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua
index 8367ec016..0d70187ba 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(57)
+test:plan(70)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -1180,4 +1180,171 @@ test:do_test(
 
     -- </table-15.1>
 })
+
+-- gh-3504 Check the possibility appear constraints along with columns
+-- definitions.
+
+test:do_execsql_test(
+    "table-21.1",
+    [[
+        CREATE TABLE t21(
+           a integer,
+           primary key (a),
+           b integer,
+           check (b > 0),
+           c integer
+           check (c > 0)
+        );
+    ]], {
+        -- <table-21.1>
+
+        -- </table-21.1>
+    })
+
+test:do_catchsql_test(
+    "table-21.2",
+    [[
+        INSERT INTO t21 VALUES(1, 1, 1);
+        INSERT INTO t21 VALUES(1, 2, 2);
+    ]], {
+        -- <table-21.2>
+        1, "Duplicate key exists in unique index 'pk_unnamed_T21_1' in space 'T21'"
+        -- </table-21.2>
+    })
+
+test:do_catchsql_test(
+    "table-21.3",
+    [[
+        INSERT INTO t21 VALUES(1, -1, 1);
+    ]], {
+        -- <table-21.3>
+        1, "CHECK constraint failed: T21"
+        -- </table-21.3>
+    })
+
+test:do_catchsql_test(
+    "table-21.4",
+    [[
+        INSERT INTO t21 VALUES(1, 1, -1);
+    ]], {
+        -- <table-21.4>
+        1, "CHECK constraint failed: T21"
+        -- </table-21.4>
+    })
+
+test:do_execsql_test(
+    "check-21.cleanup",
+    [[
+        DROP TABLE IF EXISTS t21;
+    ]], {
+        -- <check-21.cleanup>
+
+        -- </check-21.cleanup>
+    })
+
+-- gh-3504: Check the CONSTRAINT name clause can't follow a constraint
+-- only before and once or missing.
+
+test:do_catchsql_test(
+    "table-22.1",
+    [[
+        CREATE TABLE t22(
+           a integer,
+           primary key (a) constraint one
+        );
+    ]], {
+        -- <table-22.1>
+        1,"keyword \"constraint\" is reserved"
+        -- </table-22.1>
+    })
+
+test:do_execsql_test(
+    "table-22.2",
+    [[
+        CREATE TABLE t22(
+           a integer primary key,
+           b integer,
+           constraint one unique (b),
+           c integer
+        );
+    ]], {
+        -- <table-22.2>
+
+        -- </table-22.2>
+    })
+
+test:do_catchsql_test(
+    "table-22.3",
+    [[
+        INSERT INTO t22 VALUES(1, 1, 1);
+        INSERT INTO t22 VALUES(2, 1, 1);
+    ]], {
+        -- <table-22.3>
+        1,"Duplicate key exists in unique index 'unique_ONE_2' in space 'T22'"
+        -- </table-22.3>
+    })
+
+test:do_execsql_test(
+    "table-22.4",
+    [[
+        CREATE TABLE t24(
+           a integer primary key,
+           b integer constraint two unique,
+           c integer
+        );
+    ]], {
+        -- <table-22.4>
+
+        -- </table-22.4>
+    })
+
+test:do_catchsql_test(
+    "table-22.5",
+    [[
+        INSERT INTO t24 VALUES(1, 1, 1);
+        INSERT INTO t24 VALUES(2, 1, 1);
+    ]], {
+        -- <table-21.5>
+        1, "Duplicate key exists in unique index 'unique_TWO_2' in space 'T24'"
+        -- </table-22.5>
+    })
+
+test:do_catchsql_test(
+    "table-22.6",
+    [[
+        CREATE TABLE t26(
+           a integer primary key,
+           b integer constraint one constraint one unique,
+           c integer
+        );
+    ]], {
+        -- <table-22.6>
+        1,"keyword \"constraint\" is reserved"
+        -- </table-22.6>
+    })
+
+test:do_catchsql_test(
+    "table-22.7",
+    [[
+        CREATE TABLE t27(
+           a integer primary key,
+           b integer constraint one constraint two unique,
+           c integer
+        );
+    ]], {
+        -- <table-22.7>
+        1,"keyword \"constraint\" is reserved"
+        -- </table-22.7>
+    })
+
+test:do_execsql_test(
+    "check-22.cleanup",
+    [[
+        DROP TABLE IF EXISTS t22;
+        DROP TABLE IF EXISTS t24;
+    ]], {
+        -- <check-22.cleanup>
+
+        -- </check-22.cleanup>
+    })
 test:finish_test()