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 38CF02B405 for ; Wed, 15 May 2019 13:28:45 -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 US42D_AXS2GN for ; Wed, 15 May 2019 13:28:45 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E8D422B186 for ; Wed, 15 May 2019 13:28:43 -0400 (EDT) Date: Wed, 15 May 2019 20:28:40 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: do not replace the error with a syntax error Message-ID: <20190515172839.GA10603@tarantool.org> References: <76ecaa6e2ba8a69f421a73bb25fb857cf5bbe001.1557244912.git.imeevma@gmail.com> <8D8884F5-961C-477F-B5AD-8723C0BBB674@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8D8884F5-961C-477F-B5AD-8723C0BBB674@tarantool.org> 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" Cc: tarantool-patches@freelists.org Hi! Thank you for review! My answers and new patch below. On Sat, May 11, 2019 at 03:39:11PM +0300, n.pettik wrote: > > > > On 7 May 2019, at 19:03, imeevma@tarantool.org wrote: > > > > Currently, it is possible to set a syntax error, even if there has > > already been another error before. > > > > For example: > > box.execute("insert into not_exist values(1) a") > > > > The first error is "Space 'NOT_EXIST' does not exist", but "Syntax > > error near 'a'" is displayed. > > > > After this patch, all syntax errors will be set only if there have > > been no errors before. > > > > Closes #3964 > > This patch doesn’t resolve issue completely (at least what issue’s > topic says): we just ignore further errors and continue parsing process. i> Fixed. > > --- > > https://github.com/tarantool/tarantool/issues/3964 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-3964-stop-parser-on-error > > > > src/box/sql/parse.y | 12 ++++++++++++ > > test/sql-tap/e_select1.test.lua | 2 +- > > test/sql-tap/misc1.test.lua | 2 +- > > test/sql-tap/sql-errors.test.lua | 12 +++++++++++- > > test/sql-tap/view.test.lua | 2 +- > > 5 files changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > > index 3a443a0..ac3cce0 100644 > > --- a/src/box/sql/parse.y > > +++ b/src/box/sql/parse.y > > @@ -32,6 +32,18 @@ > > %syntax_error { > > UNUSED_PARAMETER(yymajor); /* Silence some compiler warnings */ > > assert( TOKEN.z[0] ); /* The tokenizer always gives us a token */ > > + /* > > + * Do nothing if there was already an error. Before this patch, > > + * a syntax error can replace an already set error when the > > + * parser rule is violated. > > + * For example: > > + * INSERT INSERT not_exist VALUES(1) a > > + * > > + * In this case space NOT_EXIST do not exist, but this error has > > + * been replaced by a syntax error. > > + */ > > + if (pParse->is_aborted) > > Let’s add assertion: > > - if (pParse->is_aborted) > + if (pParse->is_aborted) { > + assert(! diag_is_empty(diag_get())); > return; > + } > No need, since patch changes completely. > > + return; > > A bit re-phrased your comment: > > /* > * Do nothing if an error has been already set. Since we don't > * stop parsing process, syntax error can replace an already > * set error when the parser's rule is violated. > * For example: > * INSERT INSERT not_exist VALUES(1) a; > * > * In this case space NOT_EXIST doesn't exist, but this error > * will be replaced by a error 'near "''a''": syntax error' > */ > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index ac3cce0ce..44d002b4b 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -33,14 +33,14 @@ > UNUSED_PARAMETER(yymajor); /* Silence some compiler warnings */ > assert( TOKEN.z[0] ); /* The tokenizer always gives us a token */ > /* > - * Do nothing if there was already an error. Before this patch, > - * a syntax error can replace an already set error when the > - * parser rule is violated. > + * Do nothing if an error has been already set. Since we don't > + * stop parsing process, syntax error can replace an already > + * set error when the parser's rule is violated. > * For example: > - * INSERT INSERT not_exist VALUES(1) a > + * INSERT INSERT not_exist VALUES(1) a; > * > - * In this case space NOT_EXIST do not exist, but this error has > - * been replaced by a syntax error. > + * In this case space NOT_EXIST doesn't exist, but this error > + * will be replaced by a error 'near "''a''": syntax error' > */ > if (pParse->is_aborted) > No need, since patch changes completely. New patch: >From cba122be69978aa8d87fea9939fd2f37a374b689 Mon Sep 17 00:00:00 2001 Date: Wed, 15 May 2019 19:56:05 +0300 Subject: [PATCH] sql: stop parser on error This patch stops the parser if any error occurs. Prior to this patch, it was possible to replace the error with another one, since the parser may continue to work, even if an error occurred. For example: box.execute("insert into not_exist values(1) a") The first error is "Space 'NOT_EXIST' does not exist", but "Syntax error near 'a'" is displayed. After this patch, the first error will be displayed. Closes #3964 diff --git a/extra/lempar.c b/extra/lempar.c index d043e39..595f89e 100644 --- a/extra/lempar.c +++ b/extra/lempar.c @@ -935,7 +935,8 @@ void Parse( yymajor = YYNOCODE; #endif } - }while( yymajor!=YYNOCODE && yypParser->yytos>yypParser->yystack ); + }while( yymajor!=YYNOCODE && yypParser->yytos>yypParser->yystack + PARSER_ERROR_CHECK); #ifndef NDEBUG if( yyTraceFILE ){ yyStackEntry *i; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 3a443a0..f241b8d 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -80,6 +80,13 @@ #define YYMALLOCARGTYPE u64 /* + * Stop the parser if an error occurs. This macro adds an + * additional check that allows the parser to be stopped if any + * error was noticed. + */ +#define PARSER_ERROR_CHECK && ! pParse->is_aborted + +/* ** An instance of this structure holds information about the ** LIMIT clause of a SELECT statement. */ diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua index c4724e6..6f17471 100755 --- a/test/sql-tap/e_select1.test.lua +++ b/test/sql-tap/e_select1.test.lua @@ -808,7 +808,7 @@ data = { {"1.1", "SELECT a, b, c FROM z1 WHERE *", "Syntax error near '*'"}, {"1.2", "SELECT a, b, c FROM z1 GROUP BY *", "Syntax error near '*'"}, {"1.3", "SELECT 1 + * FROM z1", "Syntax error near '*'"}, - {"1.4", "SELECT * + 1 FROM z1", "Syntax error near '+'"}, + {"1.4", "SELECT * + 1 FROM z1", "Failed to expand '*' in SELECT statement without FROM clause"}, {"2.1", "SELECT *", "Failed to expand '*' in SELECT statement without FROM clause"}, {"2.2", "SELECT * WHERE 1", "Failed to expand '*' in SELECT statement without FROM clause"}, {"2.3", "SELECT * WHERE 0", "Failed to expand '*' in SELECT statement without FROM clause"}, diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua index cd0e103..914f920 100755 --- a/test/sql-tap/misc1.test.lua +++ b/test/sql-tap/misc1.test.lua @@ -1047,7 +1047,7 @@ test:do_catchsql_test( VALUES(0,0x0MATCH#0; ]], { -- - 1, [[Syntax error near ';']] + 1, [[Syntax error near '#0']] -- }) diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua index 4e173b6..83a13d2 100755 --- a/test/sql-tap/sql-errors.test.lua +++ b/test/sql-tap/sql-errors.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(45) +test:plan(46) test:execsql([[ CREATE TABLE t0 (i INT PRIMARY KEY, a INT); @@ -506,4 +506,14 @@ test:do_execsql_test( -- }) +test:do_catchsql_test( + "sql-errors-1.46", + [[ + INSERT INTO not_exist VALUES(1) a; + ]], { + -- + 1, "Space 'NOT_EXIST' does not exist" + -- + }) + test:finish_test() diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua index 0032e1b..101f4c3 100755 --- a/test/sql-tap/view.test.lua +++ b/test/sql-tap/view.test.lua @@ -961,7 +961,7 @@ test:do_catchsql_test( DROP VIEW main.nosuchview ]], { -- - 1, "Syntax error near '.'" + 1, "Space 'MAIN' does not exist" -- })