[tarantool-patches] Re: [PATCH v2 2/5] sql: rework syntax errors

["n.pettik" <korablev@tarantool.org>]

> On 25 Feb 2019, at 20:14, imeevma@tarantool.org wrote:
> 
> This patch reworks SQL syntax errors. After this patch, these
> error will be set as Tarantool errors.
> 
> Part of #3965
> ---
> src/box/errcode.h                          |  7 +++++
> src/box/sql/build.c                        |  6 ++---
> src/box/sql/parse.y                        | 41 ++++++++++++++++++------------
> src/box/sql/select.c                       | 11 ++++----
> src/box/sql/tokenize.c                     |  7 ++---
> test/box/misc.result                       |  6 +++++
> test/sql-tap/alter2.test.lua               |  4 +--
> test/sql-tap/blob.test.lua                 | 20 +++++++--------
> test/sql-tap/check.test.lua                | 10 ++++----
> test/sql-tap/colname.test.lua              |  2 +-
> test/sql-tap/count.test.lua                |  2 +-
> test/sql-tap/default.test.lua              |  4 +--
> test/sql-tap/e_select1.test.lua            | 18 ++++++-------
> test/sql-tap/gh-2367-pragma.test.lua       |  4 +--
> test/sql-tap/gh2168-temp-tables.test.lua   |  4 +--
> test/sql-tap/identifier_case.test.lua      |  2 +-
> test/sql-tap/index-info.test.lua           |  2 +-
> test/sql-tap/index1.test.lua               |  2 +-
> test/sql-tap/index7.test.lua               |  2 +-
> test/sql-tap/join.test.lua                 |  2 +-
> test/sql-tap/keyword1.test.lua             |  2 +-
> test/sql-tap/misc1.test.lua                |  8 +++---
> test/sql-tap/misc5.test.lua                |  6 ++---
> test/sql-tap/null.test.lua                 |  8 +++---
> test/sql-tap/select1.test.lua              | 20 +++++++--------
> test/sql-tap/select3.test.lua              |  4 +--
> test/sql-tap/start-transaction.test.lua    | 14 +++++-----
> test/sql-tap/table.test.lua                | 10 ++++----
> test/sql-tap/tkt3935.test.lua              | 14 +++++-----
> test/sql-tap/tokenize.test.lua             | 26 +++++++++----------
> test/sql-tap/trigger1.test.lua             | 14 +++++-----
> test/sql-tap/view.test.lua                 |  4 +--
> test/sql-tap/with1.test.lua                |  6 ++---
> test/sql-tap/with2.test.lua                | 18 ++++++-------
> test/sql/checks.result                     |  2 +-
> test/sql/collation.result                  | 12 ++++-----
> test/sql/foreign-keys.result               | 10 +++++---
> test/sql/gh-3888-values-blob-assert.result |  8 +++---
> test/sql/iproto.result                     | 10 ++++----
> test/sql/misc.result                       | 14 +++++-----
> test/sql/on-conflict.result                | 16 ++++++------
> test/sql/types.result                      | 13 ++++++----
> 42 files changed, 213 insertions(+), 182 deletions(-)
> 
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index a1fcf01..6546b2f 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -231,6 +231,13 @@ struct errcode_record {
> 	/*176 */_(ER_SQL_CANT_RESOLVE_FIELD,	"Can’t resolve field '%s'") \
> 	/*177 */_(ER_INDEX_EXISTS_IN_SPACE,	"Index '%s' already exists in space '%s'") \
> 	/*178 */_(ER_INCONSISTENT_TYPES,	"Inconsistent types: expected %s got %s") \
> +	/*179 */_(ER_SQL_SYNTAX,		"Syntax error in %s: %s") \
> +	/*180 */_(ER_SQL_STACK_OVERFLOW,	"Failed to parse SQL statement: parser stack limit reached") \
> +	/*181 */_(ER_SQL_SELECT_WILDCARD,	"Failed to expand '*' in SELECT statement without FROM clause") \
> +	/*182 */_(ER_SQL_STATEMENT_EMPTY,	"Failed to execute an empty SQL statement") \
> +	/*183 */_(ER_SQL_KEYWORD_IS_RESERVED,	"Keyword '%.*s' is reserved. Please use double quotes if '%.*s' is an identifier.") \
> +	/*184 */_(ER_SQL_SYNTAX_NEAR,		"Unrecognized syntax near '%.*s'") \

Quite strange name for err code.
I’d rather say ER_SQL_UNRECOGNIZED_SYMBOL/SYNTAX.
Is this message suggested by Konstantin? To be honest, I would
prefer old one. “Unrecognized syntax” doesn’t sound good and clear enough,
at least for me. Personally I would say “Syntax error near %s”.
The last one is used in several DBs, so I suppose it is common way to raise
errors like that.

> diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
> index e219a36..ef57955 100755
> --- a/test/sql-tap/alter2.test.lua
> +++ b/test/sql-tap/alter2.test.lua
> @@ -223,7 +223,7 @@ test:do_catchsql_test(
>         ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY REFERENCES child(id);
>     ]], {
>         -- <alter2-4.1>
> -        1, "near \"REFERENCES\": syntax error"
> +        1, "Unrecognized syntax near 'REFERENCES'"
>         -- </alter2-4.1>
>     })

The rest is obvious and OK.