[tarantool-patches] Re: [PATCH v1 1/1] sql: do not replace the error with a syntax error

[Mergen Imeev <imeevma@tarantool.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;
     ]], {
         -- <misc1-21.2>
-        1, [[Syntax error near ';']]
+        1, [[Syntax error near '#0']]
         -- </misc1-21.2>
     })
 
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(
 		-- </sql-errors-1.45>
 	})
 
+test:do_catchsql_test(
+	"sql-errors-1.46",
+	[[
+		INSERT INTO not_exist VALUES(1) a;
+	]], {
+		-- <sql-errors-1.45>
+		1, "Space 'NOT_EXIST' does not exist"
+		-- </sql-errors-1.45>
+	})
+
 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
     ]], {
         -- <view-17.2>
-        1, "Syntax error near '.'"
+        1, "Space 'MAIN' does not exist"
         -- </view-17.2>
     })