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 2F1B542F4C3 for ; Fri, 8 Nov 2019 14:01:19 +0300 (MSK) Date: Fri, 8 Nov 2019 14:01:17 +0300 From: Nikita Pettik Message-ID: <20191108110117.GA82024@tarantool.org> References: <20191006210514.49639-1-roman.habibov@tarantool.org> <20191014154750.GA21135@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH] sql: print line and column number in error message List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org On 23 Oct 13:40, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Oct 14, 2019, at 18:47, Nikita Pettik wrote: > > > > 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 > > > > ? > Oops. GitLab/Travis statuses are negative. Please check CI results before sending patch on the review. > >> 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: …” > Unfortunately, I can print line count and position only during the process > of tokenizing. Outside of the parse.y and tokenize.c we will just print the > last symbol position of a query, because this process is assumed to be > completed, as I understood. For example look at sqlSrcListAppendFromTerm(): there we set ER_SQL_SYNTAX. But it is called only from parse.y. Thus, it is possible to set correct line and position. Please, check all other errors and introduce position where it is possible. Also, taking into account my comment below, you don't need two versions (with and without position) of the same error. > >> 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. > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index ed59a875a..7546271d8 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -33,10 +33,21 @@ > UNUSED_PARAMETER(yymajor); /* Silence some compiler warnings */ > assert( TOKEN.z[0] ); /* The tokenizer always gives us a token */ > if (yypParser->is_fallback_failed && TOKEN.isReserved) { > - diag_set(ClientError, ER_SQL_KEYWORD_IS_RESERVED, TOKEN.n, TOKEN.z, > - TOKEN.n, TOKEN.z); > + if (!pParse->parse_only) { > + diag_set(ClientError, ER_SQL_KEYWORD_IS_RESERVED_WITH_POS, > + pParse->line_count, pParse->line_pos, TOKEN.n, TOKEN.z, TOKEN.n, > + TOKEN.z); > + } else { > + diag_set(ClientError, ER_SQL_KEYWORD_IS_RESERVED, TOKEN.n, TOKEN.z, > + TOKEN.n, TOKEN.z); > + } Firstly (and obviously), you can introduce wrapper around this if-else: parser_diag_set() and provide covering comment. Secondly, you can avoid using different error patterns substracting from offset length("SELECT ") (or whatever string is added to the intial query). For instance, you may set offset to the negative value from very beggining of parsing process.