[tarantool-patches] Re: [tarantool-patches] 0001-sql-COLLATE-after-LIMIT-throws-an-error.patch

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for your first patch, and see my 6 comments below!

1. Look at the letter title:  0001-sql-COLLATE-after-LIMIT-throws-an-error.patch

It does not match our convention of how to send a patch:
https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-submit-a-patch-for-review

Letter title must look like this: [PATCH] <submodule>: <description>. You can reach this
git format-patch behavior by options listed by the link above. Or use my wrapper:
https://gist.github.com/Gerold103/5471a7ddbeec346c0c845930d5bb9df4


On 15/05/2018 21:30, Мерген Имеев wrote:
>  From 102a9130f6c884411b89a33c668928e5f85422c1 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@tarantool.org>
> Date: Tue, 15 May 2018 21:25:28 +0300
> Subject: [PATCH] sql: COLLATE after LIMIT throws an error
> 
> Originally, SQLite3 execute queries with COLLATE after LIMIT like
> "SELECT * FROM test LIMIT N COLLATE not_exist"
> and queries without COLLATE like
> "SELECT * FROM test LIMIT N"
> the same way.
> 
>  From this patch on queries with COLLATE after LIMIT
> throws a syntax error.
> 
> Closes: #3010

2. Are you sure, that 'Closes: ####' actually works? I can not see ':' in
examples here: https://help.github.com/articles/closing-issues-using-keywords/

Lets better write that: Closes #3010. No ':'.

> ---
>   src/box/sql/parse.y                               | 20 +++++++++----
>   test/sql-tap/gh-2884-forbid-rowid-syntax.test.lua | 29 ++++++++++++++++++-
>   test/sql/errinj.result                            | 35 +++++++++++++++++++++++
>   test/sql/errinj.test.lua                          | 17 +++++++++++
>   4 files changed, 95 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 872647d..030d58a 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -711,11 +711,21 @@ having_opt(A) ::= HAVING expr(X).  {A = X.pExpr;}
>   //  sqlite3ExprDelete(pParse->db, $$.pOffset);
>   //}
>   limit_opt(A) ::= .                    {A.pLimit = 0; A.pOffset = 0;}
> -limit_opt(A) ::= LIMIT expr(X).       {A.pLimit = X.pExpr; A.pOffset = 0;}
> -limit_opt(A) ::= LIMIT expr(X) OFFSET expr(Y).
> -                                      {A.pLimit = X.pExpr; A.pOffset = Y.pExpr;}
> -limit_opt(A) ::= LIMIT expr(X) COMMA expr(Y).
> -                                      {A.pOffset = X.pExpr; A.pLimit = Y.pExpr;}
> +limit_opt(A) ::= LIMIT expr(X).       {
> +  if(X.pExpr->flags & EP_Collate)
> +    sqlite3ErrorMsg(pParse, "near \"COLLATE\": syntax error");

3. Verbally we have decided to move this checks into C. I see, that LIMIT and OFFSET
are evaluated here: computeLimitRegisters.
> 
>   /////////////////////////// The DELETE statement /////////////////////////////
>   //
> diff --git a/test/sql-tap/gh-2884-forbid-rowid-syntax.test.lua b/test/sql-tap/gh-2884-forbid-rowid-syntax.test.lua
> index 74d69aa..6cdc2ab 100755
> --- a/test/sql-tap/gh-2884-forbid-rowid-syntax.test.lua
> +++ b/test/sql-tap/gh-2884-forbid-rowid-syntax.test.lua
> @@ -1,6 +1,6 @@
>   #!/usr/bin/env tarantool
>   test = require("sqltester")
> -test:plan(1)
> +test:plan(4)
> 
>   local ok = pcall(test.execsql, test, [[
>       DROP TABLE IF EXISTS t1;
> @@ -9,4 +9,31 @@ local ok = pcall(test.execsql, test, [[
> 
>   test:ok(not ok, 'rowid syntax must be forbidden')
> 
> +-- gh-3010: COLLATE after LIMIT should throw an error (First part)

4. Look at the file name: gh-2884-forbid-rowid-syntax.test.lua. Looks like
intended for gh-2884 issue only. Lets find another file. For example, I found
sql-tap/collation.test.lua.
> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
> index 63d3063..1268436 100644
> --- a/test/sql/errinj.test.lua
> +++ b/test/sql/errinj.test.lua
> @@ -44,3 +44,20 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
> 
>   box.sql.execute('DROP TABLE test')
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> +

5. Error injection tests are very specific ones that use information about
Tarantool internals. Error injection is a piece of C code, that is inserted
directly into the source, and simulates various errors like WAL error, or
disk reading error, or network one. These tests are executed in DEBUG build
only.

As you can see, your test here is very out of place. Lets move it into
sql-tap/collation.test.lua too.

> +
> +cn:close()> +
> +-- gh-3010: COLLATE after LIMIT should throw an error (Second part)
> +box.sql.execute('CREATE TABLE test (id integer primary key)')
> +box.schema.user.grant('guest','read,write,execute', 'universe')
> +cn = remote.connect(box.cfg.listen)
> +cn:ping()

6. You do not need ping here.