Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org, "Мерген Имеев" <imeevma@tarantool.org>
Subject: [tarantool-patches] Re: [tarantool-patches] 0001-sql-COLLATE-after-LIMIT-throws-an-error.patch
Date: Wed, 16 May 2018 14:45:12 +0300	[thread overview]
Message-ID: <491ddba0-5088-3e85-e0c6-9c6d3d877dd0@tarantool.org> (raw)
In-Reply-To: <1526409024.918477992@f416.i.mail.ru>

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.

      reply	other threads:[~2018-05-16 11:45 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 18:30 Мерген Имеев
2018-05-16 11:45 ` Vladislav Shpilevoy [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=491ddba0-5088-3e85-e0c6-9c6d3d877dd0@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [tarantool-patches] 0001-sql-COLLATE-after-LIMIT-throws-an-error.patch' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox