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

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

> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 7764aa3..a0d8f4ae 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -122,7 +122,7 @@ struct errcode_record {
> 	/* 67 */_(ER_REPLICA_ID_IS_RESERVED,	"Can't initialize replica id with a reserved value %u") \
> 	/* 68 */_(ER_INVALID_ORDER,		"Invalid LSN order for instance %u: previous LSN = %llu, new lsn = %llu") \
> 	/* 69 */_(ER_MISSING_REQUEST_FIELD,	"Missing mandatory field '%s' in request") \
> -	/* 70 */_(ER_IDENTIFIER,		"Invalid identifier '%s' (expected printable symbols only)") \
> +	/* 70 */_(ER_IDENTIFIER,		"Invalid identifier '%s' (expected printable symbols only or it is too long)") \
> 	/* 71 */_(ER_DROP_FUNCTION,		"Can't drop function %u: %s") \
> 	/* 72 */_(ER_ITERATOR_TYPE,		"Unknown iterator type '%s'") \
> 	/* 73 */_(ER_REPLICA_MAX,		"Replica count limit reached: %u") \
> @@ -240,6 +240,13 @@ struct errcode_record {
> 	/*185 */_(ER_SQL_UNKNOWN_TOKEN,		"Syntax error: unrecognized token: '%.*s'") \
> 	/*186 */_(ER_SQL_PARSER_GENERIC,	"%s") \
> 	/*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)") \
> +	/*189 */_(ER_HEX_LITERAL_MAX,		"Hex literal %s%s length %d exceeds the supported limit (%d)") \
> +	/*190 */_(ER_INT_LITERAL_MAX,		"Integer literal %s%s exceeds the supported range %lld - %lld") \
> +	/*191 */_(ER_SQL_PARSER_LIMIT,		"%s%.*s %d exceeds the limit (%d)") \

You use auxiliary params only in one place. I guess it is worth
removing them.

> +	/*192 */_(ER_INDEX_DEF,			"%s prohibited in an index definition") \

Why not ER_UNSUPPORTED?
ER_INDEX_DEF -> ER_INDEX_DEF_UNSUPPORTED

Also, quite unnatural message pattern: it is hard to
understand diags like:

diag_set(ClientError, ER_INDEX_DEF,
        "Parameter markers are");

> +	/*193 */_(ER_CHECK_CONSTRAINT_DEF,	"%s prohibited in a CHECK constraint definition") \

The same question.
ER_CHECK_CONSTRAINT_DEF -> ER_CK_DEF_UNSUPPORTED

> +	/*194 */_(ER_PRIMARY_KEY_DEF,		"Expressions are prohibited in a primary key definition") \

Just substitute it with ER_INDEX_DEF_UNSUPPORTED:
expressions are prohibited in secondary index definition as well.

> 
> /*
>  * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0c06555..26434b1 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -267,14 +267,14 @@ int
> sqlCheckIdentifierName(Parse *pParse, char *zName)
> {
> 	ssize_t len = strlen(zName);
> -
> -	if (len > BOX_NAME_MAX || identifier_check(zName, len) != 0) {
> -		sqlErrorMsg(pParse,
> -				"identifier name is invalid: %s",
> -				zName);
> -		return SQL_ERROR;
> +	if (len <= BOX_NAME_MAX && identifier_check(zName, len) == 0)
> +		return SQL_OK;

return 0;

> +	if (len > BOX_NAME_MAX) {
> +		diag_set(ClientError, ER_IDENTIFIER,
> +			 tt_cstr(zName, BOX_INVALID_NAME_MAX));
> 	}
> -	return SQL_OK;
> +	pParse->is_aborted = true;
> +	return SQL_ERROR;

return -1;

        ssize_t len = strlen(zName);
-       if (len <= BOX_NAME_MAX && identifier_check(zName, len) == 0)
-               return SQL_OK;
        if (len > BOX_NAME_MAX) {
                diag_set(ClientError, ER_IDENTIFIER,
                         tt_cstr(zName, BOX_INVALID_NAME_MAX));
+               pParse->is_aborted = true;
+               return -1;
        }
-       pParse->is_aborted = true;
-       return SQL_ERROR;
+       if (identifier_check(zName, len) != 0) {
+               pParse->is_aborted = true;
+               return -1;
+       }
+       return 0;

> @@ -588,9 +590,9 @@ sqlAddPrimaryKey(Parse * pParse,	/* Parsing context */
> 	if (space == NULL)
> 		goto primary_key_exit;
> 	if (sql_space_primary_key(space) != NULL) {
> -		sqlErrorMsg(pParse,
> -				"table \"%s\" has more than one primary key",
> -				space->def->name);
> +		diag_set(ClientError, ER_CREATE_SPACE, space->def->name,
> +			 "too many primary keys”);

too many primary keys -> primary key already exists

> +		pParse->is_aborted = true;
> 		goto primary_key_exit;
> 	}
> 	if (pList == NULL) {
> 
> @@ -1775,6 +1778,11 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> 	}
> 	if (constraint_name == NULL)
> 		goto exit_create_fk;
> +	if (!is_self_referenced && parent_space->def->opts.is_view) {
> +		diag_set(ClientError, ER_CREATE_FK_CONSTRAINT, constraint_name,
> +			"referenced space can't be VIEW");
> +		goto tnt_error;
> +	}

This check is already processed in on_replace_dd_fk_constraint()
Could you remove this check to avoid code duplication?

> @@ -2942,11 +2958,8 @@ sqlSavepoint(Parse * pParse, int op, Token * pName)
> 			return;
> 		}
> 		if (op == SAVEPOINT_BEGIN &&
> -			sqlCheckIdentifierName(pParse, zName)
> -				!= SQL_OK) {
> -			sqlErrorMsg(pParse, "bad savepoint name");
> +		    sqlCheckIdentifierName(pParse, zName) != SQL_OK)

!= 0

> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 87d4ed4..e8a5f0d 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -157,8 +157,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> 			goto delete_from_cleanup;
> 
> 		if (trigger_list == NULL) {
> -			sqlErrorMsg(parse, "cannot modify %s because it is a"
> -					" view", space->def->name);
> +			diag_set(ClientError, ER_ALTER_SPACE, space->def->name,
> +				 "it is a view”);

Nit: it is a view -> space is a view (sounds better IMHO).

> +			parse->is_aborted = true;
> 			goto delete_from_cleanup;
> 		}
> 	}
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 4193596..de06ee0 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -750,15 +750,14 @@ codeVectorCompare(Parse * pParse,	/* Code generator context */
> int
> sqlExprCheckHeight(Parse * pParse, int nHeight)
> {
> -	int rc = SQL_OK;
> 	int mxHeight = pParse->db->aLimit[SQL_LIMIT_EXPR_DEPTH];
> 	if (nHeight > mxHeight) {
> -		sqlErrorMsg(pParse,
> -				"Expression tree is too large (maximum depth %d)",
> -				mxHeight);
> -		rc = SQL_ERROR;
> +		diag_set(ClientError, ER_SQL_PARSER_LIMIT, "Number of nodes "\
> +			 "in expression tree", 0, "", nHeight, mxHeight);
> +		pParse->is_aborted = true;
> +		return SQL_ERROR;

return -1;

> 	}
> -	return rc;
> +	return SQL_OK;

return 0;

> 
> @@ -690,9 +674,12 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
> 					 * that might change over time cannot be used
> 					 * in an index.
> 					 */
> -					notValid(pParse, pNC,
> -						 "non-deterministic functions",
> -						 NC_IdxExpr);
> +					if (pNC->ncFlags & NC_IdxExpr) {
> +						diag_set(ClientError, ER_INDEX_DEF,
> +							 "Non-deterministic functions "\
> +							 "are”);

This code seems to be unreachable: functional indexes
are filtered a way earlier.

> @@ -754,8 +741,16 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
> 			testcase(pExpr->op == TK_IN);
> 			if (ExprHasProperty(pExpr, EP_xIsSelect)) {
> 				int nRef = pNC->nRef;
> -				notValid(pParse, pNC, "subqueries",
> -					 NC_IsCheck | NC_IdxExpr);
> +				if (pNC->ncFlags & NC_IdxExpr) {
> +					diag_set(ClientError, ER_INDEX_DEF,
> +						 "Subqueries are”);

The same: managed to grep this error only for ck constraint.

> @@ -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)

> {
> 	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.

> +resolveOrderGroupBy(NameContext *pNC, Select *pSelect, ExprList *pOrderBy,
> +		    bool is_order_by)
> {
> 	int i, j;		/* Loop counters */
> 	int iCol;		/* Column number */
> 	struct ExprList_item *pItem;	/* A term of the ORDER BY clause */
> 	Parse *pParse;		/* Parsing context */
> 	int nResult;		/* Number of terms in the result set */
> +	const char *zType = is_order_by ? "ORDER" : "GROUP";
> 
> 	if (pOrderBy == 0)
> 		return 0;
> 
> 	/* Compute the limit registers */
> 	computeLimitRegisters(pParse, p, labelEnd);
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 05ceeb4..bc0ab66 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -139,8 +139,9 @@ sqlUpdate(Parse * pParse,		/* The parser context */
> 		goto update_cleanup;
> 	}
> 	if (is_view && tmask == 0) {
> -		sqlErrorMsg(pParse, "cannot modify %s because it is a view",
> -				space->def->name);
> +		diag_set(ClientError, ER_ALTER_SPACE, space->def->name,
> +			 "it is a view”);

-> space is a view

> +		pParse->is_aborted = true;
> 		goto update_cleanup;
> 	}
> 
> 
> diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
> index 0d8bf15..dd2de92 100755
> --- a/test/sql-tap/check.test.lua
> +++ b/test/sql-tap/check.test.lua
> @@ -319,7 +319,7 @@ test:do_catchsql_test(
>         );
>     ]], {
>         -- <check-3.1>
> -        1, "Failed to create space 'T3': subqueries prohibited in CHECK constraints"
> +        1, "Failed to create space 'T3': Subqueries are prohibited in a CHECK constraint definition”

Don’t use capital letter after ‘:’, it would allow you to avoid fixing tests.
The same is applied to the fixes below.

>         -- </check-3.1>
>     })
> 
> diff --git a/test/sql-tap/colname.test.lua b/test/sql-tap/colname.test.lua
> index 29fdf13..77c4280 100755
> --- a/test/sql-tap/colname.test.lua
> +++ b/test/sql-tap/colname.test.lua
> @@ -637,7 +637,7 @@ test:do_test(
> test:do_catchsql_test(
>     "colname-11.1",
>     [[ create table t1(a INT, b INT, c INT, primary key('A'))]],
> -    {1, "expressions prohibited in PRIMARY KEY"})
> +    {1, "Expressions are prohibited in a primary key definition"})
> 
> test:do_catchsql_test(
>     "colname-11.2",
> diff --git a/test/sql-tap/default.test.lua b/test/sql-tap/default.test.lua
> index 67151b0..fbe285b 100755
> --- a/test/sql-tap/default.test.lua
> +++ b/test/sql-tap/default.test.lua
> @@ -66,7 +66,7 @@ test:do_catchsql_test(
> 	);
> 	]], {
> 	-- <default-1.3>
> -	1, "default value of column [Y] is not constant"
> +	1, "Failed to create space 'T3': default value of column is not constant”

Is it possible to print also name of column (like in original version)?

> diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
> index 413dd3f..cc56592 100644
> --- a/test/sql-tap/engine.cfg
> +++ b/test/sql-tap/engine.cfg
> @@ -11,6 +11,9 @@
>     "sort.test.lua": {
>         "memtx": {"engine": "memtx"}
>     },
> +    "sql-errors.test.lua": {
> +        "memtx": {"engine": "memtx"}
> +    },

I would run this test both for vinyl and memtx.