[tarantool-patches] Re: [PATCH v1 1/1] sql: rework "no such object" and "object exists" errors

[n.pettik]

> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index f7dbb94..e25e05c 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -87,7 +87,7 @@ struct errcode_record {
> 	/* 32 */_(ER_PROC_LUA,			"%s") \
> 	/* 33 */_(ER_NO_SUCH_PROC,		"Procedure '%.*s' is not defined") \
> 	/* 34 */_(ER_NO_SUCH_TRIGGER,		"Trigger '%s' doesn't exist") \
> -	/* 35 */_(ER_NO_SUCH_INDEX,		"No index #%u is defined in space '%s'") \
> +	/* 35 */_(ER_NO_SUCH_INDEX_ID,		"No index #%u is defined in space '%s'") \
> 	/* 36 */_(ER_NO_SUCH_SPACE,		"Space '%s' does not exist") \
> 	/* 37 */_(ER_NO_SUCH_FIELD,		"Field %d was not found in the tuple") \
> 	/* 38 */_(ER_EXACT_FIELD_COUNT,		"Tuple field count %u does not match space field count %u") \
> @@ -200,12 +200,12 @@ struct errcode_record {
> 	/*145 */_(ER_NO_SUCH_SEQUENCE,		"Sequence '%s' does not exist") \
> 	/*146 */_(ER_SEQUENCE_EXISTS,		"Sequence '%s' already exists") \
> 	/*147 */_(ER_SEQUENCE_OVERFLOW,		"Sequence '%s' has overflowed") \
> -	/*148 */_(ER_UNUSED5,			"") \
> +	/*148 */_(ER_NO_SUCH_INDEX_NAME,	"No index '%s' is defined in space '%s'") \

I’d better say ’No index with name …’.
But it is to be discussed.

> 	/*149 */_(ER_SPACE_FIELD_IS_DUPLICATE,	"Space field '%s' is duplicate") \
> 	/*150 */_(ER_CANT_CREATE_COLLATION,	"Failed to initialize collation: %s.") \
> 	/*151 */_(ER_WRONG_COLLATION_OPTIONS,	"Wrong collation options (field %u): %s") \
> 	/*152 */_(ER_NULLABLE_PRIMARY,		"Primary index of the space '%s' can not contain nullable parts") \
> -	/*153 */_(ER_UNUSED,			"") \
> +	/*153 */_(ER_NO_SUCH_FIELD_NAME,	"Field '%s' doesn't exist") \

Mb it is better to split this error into two:
“Space %s doesn’t feature field with name %s” and
“Unresolved column name %s”

Again, I’m neither SQL expert nor technical writer, so it is not up to me.

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 7f17fd8..daeb02c 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3270,6 +3270,14 @@ int sqlKeywordCode(const unsigned char *, int);
> int sqlRunParser(Parse *, const char *, char **);
> 
> /**
> + * Increment error counter if error suppression isn't set.
> + *
> + * @param parse_context Current parsing context.
> + */
> +void
> +sql_parser_error(struct Parse *parse_context);

I suggest to incapsulate call of diag set inside this
function. To achieve this, lets make function take
variadic params. It would allow us to reduce size
of refactored code, at least.

> +
> +/**
>  * This routine is called after a single SQL statement has been
>  * parsed and a VDBE program to execute that statement has been
>  * prepared.  This routine puts the finishing touches on the
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index ba24283..d203604 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -191,8 +191,9 @@ sqlUpdate(Parse * pParse,		/* The parser context */
> 			}
> 		}
> 		if (j >= (int)def->field_count) {
> -			sqlErrorMsg(pParse, "no such column: %s",
> -					pChanges->a[i].zName);
> +			diag_set(ClientError, ER_NO_SUCH_FIELD_NAME,
> +				 pChanges->a[i].zName);
> +			sql_parser_error(pParse);
> 			goto update_cleanup;
> 		}
> 	}
> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index dadae18..ad7bb1e 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -246,6 +246,16 @@ sqlErrorMsg(Parse * pParse, const char *zFormat, ...)
> 	}
> }
> 
> +void
> +sql_parser_error(struct Parse *parse_context)
> +{
> +	if (parse_context->db->suppressErr)
> +		return;

As Konstantin pointed out, seems like we don’t need
taking into consideration suppressErr here.

> +	parse_context->nErr++;
> +	parse_context->rc = SQL_TARANTOOL_ERROR;
> +}
> +
> +

Extra blank line.

> diff --git a/test/sql/checks.result b/test/sql/checks.result
> index 12a3aa1..2eafae8 100644
> --- a/test/sql/checks.result
> +++ b/test/sql/checks.result
> @@ -111,11 +111,11 @@ s = box.space._space:insert(t)
> --
> box.sql.execute("CREATE TABLE w2 (s1 INT PRIMARY KEY, CHECK ((SELECT COUNT(*) FROM w2) = 0));")
> ---
> -- error: 'Failed to create space ''W2'': SQL error: no such table: W2'
> +- error: 'Failed to create space ''W2'': Space ''W2'' does not exist’

Hm, IMHO looks a bit misleading.
Could we improve error reporting in this case?
It would be nice to see smth like “name cannot be resolved” or
“Misuse of space forward declaration”.