As Alexander asked I've added table content checking in tests, fixed the causes of failing tests in Travis & fixed Nikita's remarks. Diff for the newer version is in the end.

пт, 13 июл. 2018 г. в 5:15, n.pettik <korablev@tarantool.org>:
Nitpicking ffter module prefix (i.e. sql:) don’t use upper case for first word

 
Fixed.
 
> Patch is aimed on making our sql closer to ANSI sql.

Nitpicking: write SQL in upper case - it is an abbreviation.


Fixed.
 
>
> With the patch applied only following commands can be used:
> - "START TRANSACTION" to begin transaction.
> - "COMMIT" to end transaction.
> - "ROLLBACK" to rollback transaction without savepoints.
> - "ROLLBACK TO .." to rollback transaction to savepoint.
>
> Closes #2164
> ---
>
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 990c419..1ec1538 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -120,7 +120,7 @@ static Keyword aKeywordTable[] = {
>   { "ASC",                    "TK_ASC",         ALWAYS,           true  },
>   { "AUTOINCREMENT",          "TK_AUTOINCR",    AUTOINCR,         false },
>   { "BEFORE",                 "TK_BEFORE",      TRIGGER,          false },
> -  { "BEGIN",                  "TK_BEGIN",       ALWAYS,           true  },
> +  { "BEGIN",                  "TK_BEGIN",       TRIGGER,          true  },
>   { "BETWEEN",                "TK_BETWEEN",     ALWAYS,           true  },
>   { "BY",                     "TK_BY",          ALWAYS,           true  },
>   { "CASCADE",                "TK_CASCADE",     FKEY,             false },
> @@ -210,6 +210,7 @@ static Keyword aKeywordTable[] = {
>   { "SAVEPOINT",              "TK_SAVEPOINT",   ALWAYS,           true  },
>   { "SELECT",                 "TK_SELECT",      ALWAYS,           true  },
>   { "SET",                    "TK_SET",         ALWAYS,           true  },
> +  { "START",                  "TK_START",       ALWAYS,           true  },
>   { "TABLE",                  "TK_TABLE",       ALWAYS,           true  },
>   { "THEN",                   "TK_THEN",        ALWAYS,           true  },
>   { "TO",                     "TK_TO",          ALWAYS,           true  },
> @@ -274,7 +275,6 @@ static Keyword aKeywordTable[] = {
>   { "SIGNAL",                 "TK_STANDARD",    RESERVED,         true  },
>   { "SMALLINT",               "TK_ID",          RESERVED,         true  },
>   { "SPECIFIC",               "TK_STANDARD",    RESERVED,         true  },
> -  { "START",                  "TK_STANDARD",    RESERVED,         true  },
>   { "SYSTEM",                 "TK_STANDARD",    RESERVED,         true  },
>   { "SQL",                    "TK_STANDARD",    RESERVED,         true  },
>   { "USER",                   "TK_STANDARD",    RESERVED,         true  },
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 3183e3d..42ab600 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4835,11 +4835,13 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
>        * Assert()s verify that the computation is correct.
>        */
>
> -     op = ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1);
> +     if (pExpr->op >= TK_NE && pExpr->op <= TK_GE)
> +             op = ((pExpr->op + (TK_NE & 1)) ^ 1) - (TK_NE & 1);
> +     if (pExpr->op == TK_ISNULL || pExpr->op == TK_NOTNULL)
> +             op = ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1);

Please, leave comment how this code work.
It isn't even close to be obvious, really.

Fixed.
 

>
>       /*
>        * Verify correct alignment of TK_ and OP_ constants.
> -      * Tokens TK_ISNULL and TK_NE shoud have the same parity.
>        */
>       assert(pExpr->op != TK_NE || op == OP_Eq);
>       assert(pExpr->op != TK_EQ || op == OP_Ne);
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index b2940b7..e956dfc 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -147,13 +147,11 @@ cmdx ::= cmd.
> ///////////////////// Begin and end transactions. ////////////////////////////
> //
>
> -cmd ::= BEGIN trans_opt.  {sql_transaction_begin(pParse);}
> +cmd ::= START TRANSACTION trans_opt.  {sql_transaction_begin(pParse);}
> trans_opt ::= .
> -trans_opt ::= TRANSACTION.
> -trans_opt ::= TRANSACTION nm.
> -cmd ::= COMMIT trans_opt.      {sql_transaction_commit(pParse);}
> -cmd ::= END trans_opt.         {sql_transaction_commit(pParse);}
> -cmd ::= ROLLBACK trans_opt.    {sql_transaction_rollback(pParse);}
> +trans_opt ::= nm.

What is the point of named transactions, if rollback and commit
don’t use name? I mean, now we ofc can’t use its name, but
what does ANSI say?

Fixed it.
Ansi only asks for <transaction characteristics> after 'START TRANSACTION'.
 

> +cmd ::= COMMIT.      {sql_transaction_commit(pParse);}
> +cmd ::= ROLLBACK.    {sql_transaction_rollback(pParse);}
>
> savepoint_opt ::= SAVEPOINT.
> savepoint_opt ::= .
> @@ -163,7 +161,7 @@ cmd ::= SAVEPOINT nm(X). {
> cmd ::= RELEASE savepoint_opt nm(X). {
>   sqlite3Savepoint(pParse, SAVEPOINT_RELEASE, &X);
> }
> -cmd ::= ROLLBACK trans_opt TO savepoint_opt nm(X). {
> +cmd ::= ROLLBACK TO savepoint_opt nm(X). {
>   sqlite3Savepoint(pParse, SAVEPOINT_ROLLBACK, &X);
> }
>
> diff --git a/test/sql-tap/analyze3.test.lua b/test/sql-tap/analyze3.test.lua
> index 26f8793..5079962 100755
> --- a/test/sql-tap/analyze3.test.lua
> +++ b/test/sql-tap/analyze3.test.lua
> @@ -340,7 +340,7 @@ test:do_execsql_test(
>     "analyze3-1.3.1",
>     [[
>         CREATE TABLE t3(id INTEGER PRIMARY KEY, y TEXT, x INTEGER);
> -        BEGIN;
> +        START TRANSACTION;
>           INSERT INTO t3 SELECT id, y, x FROM t1;
>         COMMIT;
>         CREATE INDEX i3 ON t3(x);
> @@ -465,7 +465,7 @@ test:do_test(
> --     function()
> --         test:execsql([[
> --             PRAGMA case_sensitive_like=off;
> ---             BEGIN;
> +--             START TRANSACTION;

I guess, you can remove it at all. It makes no sense to add dead code.

> --- a/test/sql-tap/in1.test.lua
> +++ b/test/sql-tap/in1.test.lua
> @@ -26,7 +26,7 @@ test:do_test(
>     function()
>         test:execsql [[
>             CREATE TABLE t1(a PRIMARY KEY, b);
> -            BEGIN;
> +            START TRANSACTION;
>         ]]
>         -- for _ in X(0, "X!for", [=[["set i 1","$i<=10","incr i"]]=]) do
>         local j = 1
> @@ -1073,7 +1073,7 @@ test:do_execsql_test(
> -- MUST_WORK_TEST
> -- do_test in-13.14 {
> --   execsql {
> ---     BEGIN TRANSACTION;
> +--     START TRANSACTION;

The same is here and in other places.


Fixed. 

diff --git a/extra/lempar.c b/extra/lempar.c
index 00fd79c..d043e39 100644
--- a/extra/lempar.c
+++ b/extra/lempar.c
@@ -336,8 +336,8 @@ void *ParseAlloc(void *(*mallocProc)(YYMALLOCARGTYPE)){
   if( pParser ){
 #ifdef YYTRACKMAXSTACKDEPTH
     pParser->yyhwm = 0;
-    pParser->is_fallback_failed = false;
 #endif
+    pParser->is_fallback_failed = false;
 #if YYSTACKDEPTH<=0
     pParser->yytos = NULL;
     pParser->yystack = NULL;
diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 990c419..1ec1538 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -120,7 +120,7 @@ static Keyword aKeywordTable[] = {
   { "ASC",                    "TK_ASC",         ALWAYS,           true  },
   { "AUTOINCREMENT",          "TK_AUTOINCR",    AUTOINCR,         false },
   { "BEFORE",                 "TK_BEFORE",      TRIGGER,          false },
-  { "BEGIN",                  "TK_BEGIN",       ALWAYS,           true  },
+  { "BEGIN",                  "TK_BEGIN",       TRIGGER,          true  },
   { "BETWEEN",                "TK_BETWEEN",     ALWAYS,           true  },
   { "BY",                     "TK_BY",          ALWAYS,           true  },
   { "CASCADE",                "TK_CASCADE",     FKEY,             false },
@@ -210,6 +210,7 @@ static Keyword aKeywordTable[] = {
   { "SAVEPOINT",              "TK_SAVEPOINT",   ALWAYS,           true  },
   { "SELECT",                 "TK_SELECT",      ALWAYS,           true  },
   { "SET",                    "TK_SET",         ALWAYS,           true  },
+  { "START",                  "TK_START",       ALWAYS,           true  },
   { "TABLE",                  "TK_TABLE",       ALWAYS,           true  },
   { "THEN",                   "TK_THEN",        ALWAYS,           true  },
   { "TO",                     "TK_TO",          ALWAYS,           true  },
@@ -274,7 +275,6 @@ static Keyword aKeywordTable[] = {
   { "SIGNAL",                 "TK_STANDARD",    RESERVED,         true  },
   { "SMALLINT",               "TK_ID",          RESERVED,         true  },
   { "SPECIFIC",               "TK_STANDARD",    RESERVED,         true  },
-  { "START",                  "TK_STANDARD",    RESERVED,         true  },
   { "SYSTEM",                 "TK_STANDARD",    RESERVED,         true  },
   { "SQL",                    "TK_STANDARD",    RESERVED,         true  },
   { "USER",                   "TK_STANDARD",    RESERVED,         true  },
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index b1650cf..b26728d 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4826,23 +4826,32 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
  *       TK_EQ              OP_Ne
  *       TK_GT              OP_Le
  *       TK_LE              OP_Gt
- *       TK_GE              OP_Lt
  *       TK_LT              OP_Ge
+ *       TK_GE              OP_Lt
  *        ...                ...
  *       TK_ISNULL          OP_NotNull
  *       TK_NOTNULL         OP_IsNull
  *
- * For other values of pExpr->op, op is undefined and unused.
- * The value of TK_ and OP_ constants are arranged such that we
- * can compute the mapping above using the following expression.
+ * For other values of pExpr->op, op is undefined
+ * and unused. The value of TK_ and OP_ constants
+ * are arranged such that we can compute the mapping
+ * above using the following expression. The idea
+ * is that both for OP_'s and TK_'s the first elements
+ * in the given mapping ranges of codes and tokens are
+ * 'Not equal' and 'Is null'. Moreover the 'excluding'
+ * ones (like 'Greater than' and 'Lower than or Equal')
+ * are paired and follow one each other, hence have n
+ * and n + 1 numbers.
  * Assert()s verify that the computation is correct.
  */
 
- op = ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1);
+ if (pExpr->op >= TK_NE && pExpr->op <= TK_GE)
+ op = ((pExpr->op + (TK_NE & 1)) ^ 1) - (TK_NE & 1);
+ if (pExpr->op == TK_ISNULL || pExpr->op == TK_NOTNULL)
+ op = ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1);
 
  /*
  * Verify correct alignment of TK_ and OP_ constants.
- * Tokens TK_ISNULL and TK_NE shoud have the same parity.
  */
  assert(pExpr->op != TK_NE || op == OP_Eq);
  assert(pExpr->op != TK_EQ || op == OP_Ne);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ac935fd..b4f11e5 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -147,13 +147,9 @@ cmdx ::= cmd.
 ///////////////////// Begin and end transactions. ////////////////////////////
 //
 
-cmd ::= BEGIN trans_opt.  {sql_transaction_begin(pParse);}
-trans_opt ::= .
-trans_opt ::= TRANSACTION.
-trans_opt ::= TRANSACTION nm.
-cmd ::= COMMIT trans_opt.      {sql_transaction_commit(pParse);}
-cmd ::= END trans_opt.         {sql_transaction_commit(pParse);}
-cmd ::= ROLLBACK trans_opt.    {sql_transaction_rollback(pParse);}
+cmd ::= START TRANSACTION.  {sql_transaction_begin(pParse);}
+cmd ::= COMMIT.      {sql_transaction_commit(pParse);}
+cmd ::= ROLLBACK.    {sql_transaction_rollback(pParse);}
 
 savepoint_opt ::= SAVEPOINT.
 savepoint_opt ::= .
@@ -163,7 +159,7 @@ cmd ::= SAVEPOINT nm(X). {
 cmd ::= RELEASE savepoint_opt nm(X). {
   sqlite3Savepoint(pParse, SAVEPOINT_RELEASE, &X);
 }
-cmd ::= ROLLBACK trans_opt TO savepoint_opt nm(X). {
+cmd ::= ROLLBACK TO savepoint_opt nm(X). {
   sqlite3Savepoint(pParse, SAVEPOINT_ROLLBACK, &X);
 }
diff --git a/test/sql-tap/start-transaction.test.lua b/test/sql-tap/start-transaction.test.lua
new file mode 100755
index 0000000..eece4cd
--- /dev/null
+++ b/test/sql-tap/start-transaction.test.lua
@@ -0,0 +1,266 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(21)
+
+test:do_catchsql_test(
+ "start-transaction-1.0",
+ [[
+ CREATE TABLE IF NOT EXISTS t(id int PRIMARY KEY);
+ DELETE FROM t;
+ BEGIN;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.0>
+ 1, "near \"BEGIN\": syntax error"
+ -- <start-transaction-1.0>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.1",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.1>
+
+ -- <start-transaction-1.1>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.2",
+ [[
+ CREATE TABLE IF NOT EXISTS t(id int primary key);
+ delete from t;
+ BEGIN TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.1>
+ 1, "near \"BEGIN\": syntax error"
+ -- <start-transaction-1.1>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.3",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.3>
+
+ -- <start-transaction-1.3>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.4",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.4>
+ 0
+ -- <start-transaction-1.4>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.5",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.5>
+ 1, 2
+ -- <start-transaction-1.5>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.6",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT TRANSACTION;
+ ]], {
+ -- <start-transaction-1.6>
+ 1, "keyword \"TRANSACTION\" is reserved"
+ -- <start-transaction-1.6>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.7",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.7>
+ 1, 2
+ -- <start-transaction-1.7>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.8",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ END;
+ ]], {
+ -- <start-transaction-1.8>
+ 1, "keyword \"END\" is reserved"
+ -- <start-transaction-1.8>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.9",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.9>
+ 1, 2
+ -- <start-transaction-1.9>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.10",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ END TRANSACTION;
+ ]], {
+ -- <start-transaction-1.10>
+ 1, "keyword \"END\" is reserved"
+ -- <start-transaction-1.10>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.11",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.11>
+ 1, 2
+ -- <start-transaction-1.11>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.12",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ ROLLBACK;
+ ]], {
+ -- <start-transaction-1.12>
+ 0
+ -- <start-transaction-1.12>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.13",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.13>
+
+ -- <start-transaction-1.13>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.14",
+ [[
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ ROLLBACK TRANSACTION;
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.14>
+ 1, "keyword \"TRANSACTION\" is reserved"
+ -- <start-transaction-1.14>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.15",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.15>
+ 1, 2
+ -- <start-transaction-1.15>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.16",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ SAVEPOINT s1;
+ INSERT INTO t VALUES (2);
+ ROLLBACK TO s1;
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.16>
+ 0
+ -- <start-transaction-1.16>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.17",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.17>
+ 1
+ -- <start-transaction-1.17>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.18",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ SAVEPOINT s1;
+ INSERT INTO t VALUES (2);
+ ROLLBACK TRANSACTION TO s1;
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.18>
+ 1, "keyword \"TRANSACTION\" is reserved"
+ -- <start-transaction-1.18>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.19",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.19>
+ 1, 2
+ -- <start-transaction-1.19>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.20",
+ [[
+ drop table t;
+ ]], {
+ -- <start0transaction-1.20>
+
+ -- <start-transaction-1.20>
+ })
+
+test:finish_test()