[tarantool-patches] Re: [PATCH v4 7/8] sql: rework semantic errors

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4453 bytes --]


>>> @@ -1042,27 +1044,34 @@ resolveCompoundOrderBy(Parse * pParse,	/* Parsing context.  Leave error messages
>>> * field) then convert that term into a copy of the corresponding result set
>>> * column.
>>> *
>>> - * If any errors are detected, add an error message to pParse and
>>> - * return non-zero.  Return zero if no errors are seen.
>>> + * @param pParse Parsing context.
>>> + * @param pSelect The SELECT statement containing the clause.
>>> + * @param pOrderBy The ORDER BY or GROUP BY clause to be
>>> + *                 processed.
>>> + * @param is_order_by True if pOrderBy is ORDER BY, false if
>>> + *                    pOrderBy is GROUP BY
>>> + * @retval 0 On success, not 0 elsewhere.
>>> */
>>> int
>>> -sqlResolveOrderGroupBy(Parse * pParse,	/* Parsing context.  Leave error messages here */
>>> -			   Select * pSelect,	/* The SELECT statement containing the clause */
>>> -			   ExprList * pOrderBy,	/* The ORDER BY or GROUP BY clause to be processed */
>>> -			   const char *zType	/* "ORDER" or "GROUP" */
>>> -    )
>>> +sqlResolveOrderGroupBy(Parse *pParse, Select *pSelect, ExprList *pOrderBy,
>>> +		       bool is_order_by)
>> 
>> Why did you decide to fix code style here? Anyway, you didn't finished it
>> (struct prefixes, param naming and so on and so forth)
>> 
> I fixed this because I thought that it is quite unreliable to
> differentiate names of term using first letter of its name. Should
> I remove these changes?

But below you anyway get string representation. So, let’s return
back previous version.

>>> {
>>> 	int i;
>>> 	sql *db = pParse->db;
>>> 	ExprList *pEList;
>>> 	struct ExprList_item *pItem;
>>> +	const char *zType = is_order_by ? "ORDER" : "GROUP";
>>> 
>>> 	if (pOrderBy == 0 || pParse->db->mallocFailed)
>>> 		return 0;
>>> 
>>> @@ -1096,22 +1105,23 @@ sqlResolveOrderGroupBy(Parse * pParse,	/* Parsing context.  Leave error messages
>>> * result-set expression.  Otherwise, the expression is resolved in
>>> * the usual way - using sqlResolveExprNames().
>>> *
>>> - * This routine returns the number of errors.  If errors occur, then
>>> - * an appropriate error message might be left in pParse.  (OOM errors
>>> - * excepted.)
>>> + * @param pNC The name context of the SELECT statement.
>>> + * @param pSelect The SELECT statement containing the clause.
>>> + * @param pOrderBy An ORDER BY or GROUP BY clause to resolve.
>>> + * @param is_order_by True if pOrderBy is ORDER BY, false if
>>> + *                    pOrderBy is GROUP BY
>>> + * @retval 0 On success, not 0 elsewhere.
>>> */
>>> static int
>>> -resolveOrderGroupBy(NameContext * pNC,	/* The name context of the SELECT statement */
>>> -		    Select * pSelect,	/* The SELECT statement holding pOrderBy */
>>> -		    ExprList * pOrderBy,	/* An ORDER BY or GROUP BY clause to resolve */
>>> -		    const char *zType	/* Either "ORDER" or "GROUP", as appropriate */
>>> -    )
>> 
>> The same question.
>> 
> Answer above.
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0022254..b6b6c24 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -603,7 +603,7 @@ sqlAddPrimaryKey(Parse * pParse,	/* Parsing context */
> 		goto primary_key_exit;
> 	if (sql_space_primary_key(space) != NULL) {
> 		diag_set(ClientError, ER_CREATE_SPACE, space->def->name,
> -			 "too many primary keys");
> +			 "primary key already exists”);

Sorry, could you fix message to “primary key has been already declared”
or “can’t declare PRIMARY KEY more than once”? This error is related
only to CREATE TABLE processing, so nothing is created at this stage yet.
Hence, error msg may seem a little bit misleading.

> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index de0f282..efb895f 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -481,8 +481,8 @@ sqlRunParser(Parse * pParse, const char *zSql)
> 				      &pParse->sLastToken.isReserved);
> 			i += pParse->sLastToken.n;
> 			if (i > mxSqlLen) {
> -				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> -					 "string or blob too big");
> +				diag_set(ClientError, ER_SQL_PARSER_LIMIT,
> +					 "SQL command length", i, mxSqlLen);
> 				pParse->is_aborted = true;
> 				break;
> 			}
> 
> I will investigate this error a bit later.

Ok, then file it (and the rest of comments which you haven’t fixed).



[-- Attachment #2: Type: text/html, Size: 52560 bytes --]