[Tarantool-patches] [PATCH] sql: print line and column number in error message
Nikita Pettik
korablev at tarantool.org
Mon Oct 14 18:47:52 MSK 2019
On 07 Oct 00:05, Roman Khabibov wrote:
> Count the number of rows and columns during query parsing. This is
> to make it easier for the user to find errors in multiline
> queries.
>
> Closes #2611
>
> # Please enter the commit message for your changes. Lines starting
?
> ---
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index d5d396d87..36e5c179f 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -235,9 +235,9 @@ struct errcode_record {
> /*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_UNRECOGNIZED_SYNTAX, "Syntax error near '%.*s'") \
> - /*185 */_(ER_SQL_UNKNOWN_TOKEN, "Syntax error: unrecognized token: '%.*s'") \
> + /*183 */_(ER_SQL_KEYWORD_IS_RESERVED, "Keyword '%.*s' on line %d at character %d is reserved. Please use double quotes if '%.*s' is an identifier.") \
> + /*184 */_(ER_SQL_UNRECOGNIZED_SYNTAX, "Syntax error on line %d at character %d near '%.*s'") \
> + /*185 */_(ER_SQL_UNKNOWN_TOKEN, "Syntax error: on line %d at character %d unrecognized token: '%.*s'") \
> /*186 */_(ER_SQL_PARSER_GENERIC, "%s") \
What about other syntax related errors like ER_SQL_PARSER_GENERIC,
ER_SQL_SYNTAX? Please use the same pattern for all errors:
"On line %d at/near position %d ..." OR
"Syntax error on line %d at column %d: ..."
> /*187 */_(ER_SQL_ANALYZE_ARGUMENT, "ANALYZE statement argument %s is not a base table") \
> /*188 */_(ER_SQL_COLUMN_COUNT_MAX, "Failed to create space '%s': space column count %d exceeds the limit (%d)") \
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index e077a8b5e..d23feba96 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -243,6 +243,8 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
> memset(parser, 0, sizeof(struct Parse));
> parser->db = db;
> parser->sql_flags = sql_flags;
> + parser->line_count = 1;
> + parser->offset_count = 1;
> region_create(&parser->region, &cord()->slabc);
> }
>
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index fbb39878a..89839e046 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2172,6 +2172,8 @@ struct Parse {
> ***********************************************************************/
>
> Token sLastToken; /* The last token parsed */
> + int line_count; /* The line counter. */
> + int offset_count; /* The offset counter. */
Nit: use uint32_t type; use proper commenting style (/**...*/ above
the subject of comment); offset_count -> offset_pos/line_pos
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 9fa069d09..ee3978f3a 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -192,12 +193,20 @@ sql_token(const char *z, int *type, bool *is_reserved)
> i = 1 + sql_skip_spaces(z+1);
> *type = TK_SPACE;
> return i;
> + case CC_LINEFEED:
> + *type = TK_LINEFEED;
> + return 1;
> case CC_MINUS:
> if (z[1] == '-') {
> - for (i = 2; (c = z[i]) != 0 && c != '\n'; i++) {
> + for (i = 2; true; i++) {
> + if (z[i] == '\0') {
> + *type = TK_SPACE;
Please add a comment explaining that here we are ignoring comment
till the end of string and add test case.
> diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
> index e0bd60727..56030e1f8 100755
> --- a/test/sql-tap/alter2.test.lua
> +++ b/test/sql-tap/alter2.test.lua
> diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua
> index 03b1054d2..b43e30876 100755
> --- a/test/sql-tap/keyword1.test.lua
> +++ b/test/sql-tap/keyword1.test.lua
> @@ -238,12 +238,61 @@ end
>
> for _, kw in ipairs(bannedkws) do
> query = 'CREATE TABLE '..kw..'(a INT PRIMARY KEY);'
> - test:do_catchsql_test(
> + if kw == 'end' then
> + test:do_catchsql_test(
> "bannedkw1-"..kw..".1",
> query, {
> - 1, "Keyword '"..kw.."' is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + 1, "Keyword '"..kw.."' on line 1 at character 17 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> })
Why not base_len + string.len(kw)? These if-else's look extremely awful.
> + elseif kw == 'match' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 19 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + elseif kw == 'release' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 21 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + elseif kw == 'rename' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 20 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + elseif kw == 'replace' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 21 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + elseif kw == 'binary' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 20 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + elseif kw == 'character' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 23 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + elseif kw == 'smallint' then
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 22 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + else
> + test:do_catchsql_test(
> + "bannedkw1-"..kw..".1",
> + query, {
> + 1, "Keyword '"..kw.."' on line 1 at character 14 is reserved. Please use double quotes if '"..kw.."' is an identifier."
> + })
> + end
> end
>
> -
> test:finish_test()
> diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua
> index b84093e3c..cc61016ac 100755
> --- a/test/sql-tap/misc1.test.lua
> +++ b/test/sql-tap/misc1.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(58)
> +test:plan(59)
>
> --!./tcltestrunner.lua
> -- 2001 September 15.
> @@ -262,17 +262,26 @@ test:do_execsql_test(
> -- before executing a command. Thus if "WHERE" is misspelled on an UPDATE,
> -- the user won't accidently update every record.
> --
> -test:do_catchsql_test(
> - "misc1-5.1",
> +
> +test:do_execsql_test(
> + "misc1-5.1.1",
> [[
> CREATE TABLE t3(a INT primary key,b INT );
> INSERT INTO t3 VALUES(1,2);
> INSERT INTO t3 VALUES(3,4);
> + ]], {
> + -- <misc1-5.1.1>
> + -- </misc1-5.1.1>
Originally, these comments were generated automatically by tcl to Lua
converter. You don't have to put them to each test's result :)
> + })
> +
> + 1, "Syntax error on line 1 at character 60 near ';'"
> -- </6.8>
> })
>
> @@ -587,7 +587,7 @@ test:do_catchsql_test(6.9, [[
> WITH x AS (SELECT * FROM t1) UPDATE t2 SET a = 10, b = 1 WHERE a===b;
> ]], {
> -- <6.9>
> - 1, "Syntax error near '='"
> + 1, "Syntax error on line 1 at character 71 near '='"
> -- </6.9>
> })
>
> diff --git a/test/sql/checks.result b/test/sql/checks.result
> index 50347bc3a..4ecab9ad4 100644
> --- a/test/sql/checks.result
> +++ b/test/sql/checks.result
> @@ -39,8 +39,8 @@ _ = box.space.test:create_index('pk')
> -- Invalid expression test.
> box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5'})
> ---
> -- error: 'Failed to create check constraint ''CK_CONSTRAINT_01'': Syntax error near
> - ''<'''
> +- error: 'Failed to create check constraint ''CK_CONSTRAINT_01'': Syntax error on
> + line 1 at character 10 near ''<'''
There's parse_only flag in struct Parse. You can use it to calculate
correct position in expression.
More information about the Tarantool-patches
mailing list