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

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

[imeevma]

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
---
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)
+    return;
   if (yypParser->is_fallback_failed && TOKEN.isReserved) {
     diag_set(ClientError, ER_SQL_KEYWORD_IS_RESERVED, TOKEN.n, TOKEN.z,
              TOKEN.n, TOKEN.z);
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>
     })
 
-- 
2.7.4

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

[n.pettik]

> 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.

> ---
> 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;
+  }

> +    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)

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

[Mergen Imeev]

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>
     })
 

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

[Mergen Imeev]

I found that this patch also solves issue #4195. I added a new
test and fixed a commit message. New patch below.

On Wed, May 15, 2019 at 08:28:39PM +0300, Mergen Imeev wrote:
> 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 4be49385c22bcf9f31d0cb342a9bf493c7ef1493 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
Closes #4195

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..9357c40 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(47)
 
 test:execsql([[
 	CREATE TABLE t0 (i INT PRIMARY KEY, a INT);
@@ -506,4 +506,24 @@ 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.46>
+		1, "Space 'NOT_EXIST' does not exist"
+		-- </sql-errors-1.46>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.47",
+	[[
+		DROP TABLE IF EXISTS END;
+	]], {
+		-- <sql-errors-1.47>
+		1, "Keyword 'END' is reserved. Please use double quotes if 'END' is an identifier."
+		-- </sql-errors-1.47>
+	})
+
 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>
     })
 
 

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]


> 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

Any arguments in favour of using macro in this case?
Why don’t simply inline this condition?
The rest is OK as obvious.


[-- Attachment #2: Type: text/html, Size: 20498 bytes --]

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

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]


On 5/15/19 10:28 PM, n.pettik wrote:
>
>> 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
>
> Any arguments in favour of using macro in this case?
> Why don’t simply inline this condition?
> The rest is OK as obvious.
>
Currently, pParse is not used directly in a template (lempar.c).
I tried to keep this practice.

[-- Attachment #2: Type: text/html, Size: 26460 bytes --]

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]



> On 15 May 2019, at 22:33, Imeev Mergen <imeevma@tarantool.org> wrote:
> On 5/15/19 10:28 PM, n.pettik wrote:
>> 
>>> 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
>> 
>> Any arguments in favour of using macro in this case?
>> Why don’t simply inline this condition?
>> The rest is OK as obvious.
>> 
> Currently, pParse is not used directly in a template (lempar.c).
> I tried to keep this practice.

OK, then LGTM.

[-- Attachment #2: Type: text/html, Size: 26810 bytes --]

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

[Kirill Yukhin]

Hello,

On 07 May 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
> ---
> https://github.com/tarantool/tarantool/issues/3964
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3964-stop-parser-on-error

I've checked your patch into master.

--
Regards, Kirill Yukhin