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 66DD624B3A for ; Wed, 16 May 2018 07:45:17 -0400 (EDT) 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 RLa2P2WkdwIG for ; Wed, 16 May 2018 07:45:17 -0400 (EDT) Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 204EF23562 for ; Wed, 16 May 2018 07:45:15 -0400 (EDT) Subject: [tarantool-patches] Re: [tarantool-patches] 0001-sql-COLLATE-after-LIMIT-throws-an-error.patch References: <1526409024.918477992@f416.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <491ddba0-5088-3e85-e0c6-9c6d3d877dd0@tarantool.org> Date: Wed, 16 May 2018 14:45:12 +0300 MIME-Version: 1.0 In-Reply-To: <1526409024.918477992@f416.i.mail.ru> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org, =?UTF-8?B?0JzQtdGA0LPQtdC9INCY0LzQtdC1?= =?UTF-8?B?0LI=?= 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] : . 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 > 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.