From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F1D6D43D678 for ; Mon, 14 Oct 2019 18:47:54 +0300 (MSK) Date: Mon, 14 Oct 2019 18:47:52 +0300 From: Nikita Pettik Message-ID: <20191014154750.GA21135@tarantool.org> References: <20191006210514.49639-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191006210514.49639-1-roman.habibov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: print line and column number in error message List-Id: Tarantool development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@freelists.org, tarantool-patches@dev.tarantool.org 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); > + ]], { > + -- > + -- 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 ';'" > -- > }) > > @@ -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 '='" > -- > }) > > 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.