Re: [Tarantool-patches] [tarantool-patches] [PATCH] sql: print line and column number in error message

[Nikita Pettik <korablev@tarantool.org>]

On 23 Oct 13:40, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
> > On Oct 14, 2019, at 18:47, Nikita Pettik <korablev@tarantool.org> 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.