[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