From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D37D22F2CF for ; Wed, 21 Nov 2018 19:24:00 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Vocucd0UEv-J for ; Wed, 21 Nov 2018 19:24:00 -0500 (EST) Received: from forward501j.mail.yandex.net (forward501j.mail.yandex.net [5.45.198.251]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 42B152BC6E for ; Wed, 21 Nov 2018 19:24:00 -0500 (EST) From: roman.habibov1@yandex.ru In-Reply-To: <0E11C6B2-9BF0-411F-80F8-AA20A1DF63FB@tarantool.org> References: <20181118143151.24016-1-roman.habibov1@yandex.ru> <8d84eb1f-95c4-0043-abc7-dc19f663a0df@tarantool.org> <1653371542776687@myt6-add70abb4f02.qloud-c.yandex.net> <0E11C6B2-9BF0-411F-80F8-AA20A1DF63FB@tarantool.org> Subject: [tarantool-patches] Re: [PATCH] sql: constraints definition among columns in CREATE TABLE() MIME-Version: 1.0 Date: Thu, 22 Nov 2018 03:23:57 +0300 Message-Id: <4023561542846237@sas2-2074c606c35d.qloud-c.yandex.net> Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , "tarantool-patches@freelists.org" Cc: Vladislav Shpilevoy Hi! Thanks for review. > Commit message lacks a verb. I would call it sort of: > > sql: allow appearing constraint definition among columns Done. >>     Allow constraints to appear along with columns definitions. Disallow typing >>     a constraint name without specifying the constraint and after. > > Please, re-phrase last sentence, it is really hard to understand what does it mean: > after what? Done. > You can fix several nitpickings below, but even with them patch is OK. > >>  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( >>          -- >>      }) >> >>  -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 > > This test is copy of previous one. To diversify cases you can check > another type of constraint. For example: > > CREATE TABLE t (id INT PRIMARY KEY CONSTRAINT PK); > > Also, both your examples fail even without your patch since table > lacks primary key (yes, error message is different, but lets make > only one fail in this test). Added primary keys. I thought this file about CHECK CONSTRAINT. >>  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( >> >>      -- >>  }) >>  + >>  +-- gh-3504 Check the possibility appear constraints along with columns >>  +-- definitions. > > Re-phrase like: “Constraints definition can appear among columns ones." Done. >>  + >>  +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) >>  + ); > > It would be better to use uppercase for SQL keywords. Done. >>  + ]], { >>  + -- >>  + >>  + -- >>  + }) >>  + >>  +test:do_catchsql_test( >>  + "table-21.2", >>  + [[ >>  + INSERT INTO t21 VALUES(1, 1, 1); >>  + INSERT INTO t21 VALUES(1, 2, 2); >>  + ]], { >>  + -- >>  + 1, "Duplicate key exists in unique index 'pk_unnamed_T21_1' in space 'T21'" >>  + -- >>  + }) >>  + >>  +test:do_catchsql_test( >>  + "table-21.3", >>  + [[ >>  + INSERT INTO t21 VALUES(1, -1, 1); >>  + ]], { >>  + -- >>  + 1, "CHECK constraint failed: T21" >>  + -- >>  + }) >>  + >>  +test:do_catchsql_test( >>  + "table-21.4", >>  + [[ >>  + INSERT INTO t21 VALUES(1, 1, -1); >>  + ]], { >>  + -- >>  + 1, "CHECK constraint failed: T21" >>  + -- >>  + }) >>  + >>  +test:do_execsql_test( >>  + "check-21.cleanup", >>  + [[ >>  + DROP TABLE IF EXISTS t21; >>  + ]], { >>  + -- >>  + >>  + -- >>  + }) >>  + >>  +-- gh-3504: Check the CONSTRAINT name clause can't follow a constraint >>  +-- only before and once or missing. > > Can’t parse this sentence as well. Re-phrase it pls. + +-- gh-3504: Check the CONSTRAINT name clause can't follow a constraint. +-- The name may be typed once before the constraint or not. + >>  + >>  +test:do_catchsql_test( >>  + "table-22.1", >>  + [[ >>  + CREATE TABLE t22( >>  + a integer, >>  + primary key (a) constraint one >>  + ); >>  + ]], { >>  + -- >>  + 1,"keyword \"constraint\" is reserved" >>  + -- >>  + }) >>  + >>  +test:do_execsql_test( >>  + "table-22.2", >>  + [[ >>  + CREATE TABLE t22( >>  + a integer primary key, >>  + b integer, >>  + constraint one unique (b), >>  + c integer >>  + ); >>  + ]], { > > Also, I would add test where several constraint definitions > come one after another, like this: > > CREATE TABLE t (id INT, PRIMARY KEY (id), CONSTRAINT pk1 CHECK(id != 0), CONSTRAINT pk2 CHECK(id > 10)); +test:do_execsql_test( + "table-22.8", + [[ + CREATE TABLE T28( + id INT, + PRIMARY KEY (id), + CONSTRAINT check1 CHECK(id != 0), + CONSTRAINT check2 CHECK(id > 10) + ); + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "table-22.9", + [[ + INSERT INTO T28 VALUES(11); + INSERT INTO T28 VALUES(11); + ]], { + -- + 1,"Duplicate key exists in unique index 'pk_unnamed_T28_1' in space 'T28'" + -- + }) + +test:do_catchsql_test( + "table-22.10", + [[ + INSERT INTO T28 VALUES(0); + ]], { + -- + 1, "CHECK constraint failed: CHECK1" + -- + }) + +test:do_catchsql_test( + "table-22.11", + [[ + INSERT INTO T28 VALUES(9); + ]], { + -- + 1, "CHECK constraint failed: CHECK2" + -- + }) + commit 0cd2b50906b21d37c017b5e8468e13afb7a7c65b Author: Roman Khabibov Date: Sun Nov 18 15:54:53 2018 +0300 sql: allow appearing constraint definition among columns Allow constraints to appear along with columns definitions. Disallow typing a constraint name without specifying the constraint and after it. 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..c24ae2ff1 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,33 @@ test:do_catchsql_test( -- }) --- 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 + PRIMARY KEY (x) ); ]], { -- - + 1,"near \",\": syntax error" -- }) test:do_catchsql_test( "check-2.11", - [[ - INSERT INTO t2b VALUES('xyzzy','hi',5); - ]], { - -- - 1, "CHECK constraint failed: T2B" - -- - }) - -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, + PRIMARY KEY (x) ); ]], { - -- - - -- - }) - -test:do_catchsql_test( - "check-2.13", - [[ - INSERT INTO t2c VALUES('xyzzy',7,8); - ]], { - -- - 1, "CHECK constraint failed: X_TWO" - -- + -- + 1,"near \",\": syntax error" + -- }) test:do_execsql_test( diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua index 8367ec016..7fd9bac9f 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(74) --!./tcltestrunner.lua -- 2001 September 15 @@ -1180,4 +1180,217 @@ test:do_test( -- }) + +-- gh-3504 Constraints definition can appear among columns ones. + +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) + ); + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "table-21.2", + [[ + INSERT INTO T21 VALUES(1, 1, 1); + INSERT INTO T21 VALUES(1, 2, 2); + ]], { + -- + 1, "Duplicate key exists in unique index 'pk_unnamed_T21_1' in space 'T21'" + -- + }) + +test:do_catchsql_test( + "table-21.3", + [[ + INSERT INTO T21 VALUES(1, -1, 1); + ]], { + -- + 1, "CHECK constraint failed: T21" + -- + }) + +test:do_catchsql_test( + "table-21.4", + [[ + INSERT INTO T21 VALUES(1, 1, -1); + ]], { + -- + 1, "CHECK constraint failed: T21" + -- + }) + +test:do_execsql_test( + "check-21.cleanup", + [[ + DROP TABLE IF EXISTS T21; + ]], { + -- + + -- + }) + +-- gh-3504: Check the CONSTRAINT name clause can't follow a constraint. +-- The name may be typed once before the constraint or not. + +test:do_catchsql_test( + "table-22.1", + [[ + CREATE TABLE T22( + A INTEGER, + PRIMARY KEY (A) CONSTRAINT ONE + ); + ]], { + -- + 1,"keyword \"CONSTRAINT\" is reserved" + -- + }) + +test:do_execsql_test( + "table-22.2", + [[ + CREATE TABLE T22( + A INTEGER PRIMARY KEY, + B INTEGER, + CONSTRAINT ONE UNIQUE (B), + C INTEGER + ); + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "table-22.3", + [[ + INSERT INTO T22 VALUES(1, 1, 1); + INSERT INTO T22 VALUES(2, 1, 1); + ]], { + -- + 1,"Duplicate key exists in unique index 'unique_ONE_2' in space 'T22'" + -- + }) + +test:do_execsql_test( + "table-22.4", + [[ + CREATE TABLE T24( + A INTEGER PRIMARY KEY, + B INTEGER CONSTRAINT TWO UNIQUE, + C INTEGER + ); + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "table-22.5", + [[ + INSERT INTO T24 VALUES(1, 1, 1); + INSERT INTO T24 VALUES(2, 1, 1); + ]], { + -- + 1, "Duplicate key exists in unique index 'unique_TWO_2' in space 'T24'" + -- + }) + +test:do_catchsql_test( + "table-22.6", + [[ + CREATE TABLE T26( + A INTEGER PRIMARY KEY, + B INTEGER CONSTRAINT ONE CONSTRAINT ONE UNIQUE, + C INTEGER + ); + ]], { + -- + 1,"keyword \"CONSTRAINT\" is reserved" + -- + }) + +test:do_catchsql_test( + "table-22.7", + [[ + CREATE TABLE T27( + A INTEGER PRIMARY KEY, + B INTEGER CONSTRAINT ONE CONSTRAINT TWO UNIQUE, + C INTEGER + ); + ]], { + -- + 1,"keyword \"CONSTRAINT\" is reserved" + -- + }) + +test:do_execsql_test( + "table-22.8", + [[ + CREATE TABLE T28( + id INT, + PRIMARY KEY (id), + CONSTRAINT check1 CHECK(id != 0), + CONSTRAINT check2 CHECK(id > 10) + ); + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "table-22.9", + [[ + INSERT INTO T28 VALUES(11); + INSERT INTO T28 VALUES(11); + ]], { + -- + 1,"Duplicate key exists in unique index 'pk_unnamed_T28_1' in space 'T28'" + -- + }) + +test:do_catchsql_test( + "table-22.10", + [[ + INSERT INTO T28 VALUES(0); + ]], { + -- + 1, "CHECK constraint failed: CHECK1" + -- + }) + +test:do_catchsql_test( + "table-22.11", + [[ + INSERT INTO T28 VALUES(9); + ]], { + -- + 1, "CHECK constraint failed: CHECK2" + -- + }) + +test:do_execsql_test( + "check-22.cleanup", + [[ + DROP TABLE IF EXISTS t22; + DROP TABLE IF EXISTS t24; + DROP TABLE IF EXISTS t28; + ]], { + -- + + -- + }) test:finish_test()