Re: [PATCH v2 3/4] sql: use space_by_name in SQL

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Thu, Oct 25, 2018 at 11:17:11AM +0300, Kirill Yukhin wrote:
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 3d72e31..7c28437 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -47,18 +47,17 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab,
>  	if (new_name == NULL)
>  		goto exit_rename_table;
>  	/* Check that new name isn't occupied by another table. */
> -	uint32_t space_id = box_space_id_by_name(new_name, strlen(new_name));
> -	if (space_id != BOX_ID_NIL) {
> +	struct space *space = space_by_name(new_name);
> +	if (space != NULL) {
>  		diag_set(ClientError, ER_SPACE_EXISTS, new_name);
>  		goto tnt_error;
>  	}
>  	const char *tbl_name = src_tab->a[0].zName;
> -	space_id = box_space_id_by_name(tbl_name, strlen(tbl_name));
> -	if (space_id == BOX_ID_NIL) {
> +	space = space_by_name(tbl_name);
> +	if (space == NULL) {
>  		diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
>  		goto tnt_error;
>  	}
> -	struct space *space = space_by_id(space_id);
>  	assert(space != NULL);

Pointless assertion.

>  	if (space->def->opts.is_view) {
>  		diag_set(ClientError, ER_ALTER_SPACE, tbl_name,
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 674d53d..ad068a54 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1114,16 +1114,15 @@ sqlite3Analyze(Parse * pParse, Token * pName)
>  		/* Form 2:  Analyze table named */
>  		char *z = sqlite3NameFromToken(db, pName);
>  		if (z != NULL) {
> -			uint32_t space_id = box_space_id_by_name(z, strlen(z));
> -			if (space_id != BOX_ID_NIL) {
> -				struct space *sp = space_by_id(space_id);
> -				assert(sp != NULL);
> -				if (sp->def->opts.is_view) {
> +			struct space *space = space_by_name(z);
> +			if (space != NULL) {
> +				assert(space != NULL);

Another one.

> +				if (space->def->opts.is_view) {
>  					sqlite3ErrorMsg(pParse, "VIEW isn't "\
>  							"allowed to be "\
>  							"analyzed");
>  				} else {
> -					vdbe_emit_analyze_table(pParse, sp);
> +					vdbe_emit_analyze_table(pParse, space);
>  				}
>  			} else {
>  				diag_set(ClientError, ER_NO_SUCH_SPACE, z);
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 8f6c76f..72e0575 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -36,15 +36,14 @@
>  #include "tarantoolInt.h"
>  
>  struct Table *
> -sql_lookup_table(struct Parse *parse, struct SrcList_item *tbl_name)
> +sql_lookup_table(struct Parse *parse, struct SrcList_item *src_name)
>  {
> -	assert(tbl_name != NULL);
> -	assert(tbl_name->pTab == NULL);
> -	uint32_t space_id = box_space_id_by_name(tbl_name->zName,
> -						 strlen(tbl_name->zName));
> -	struct space *space = space_by_id(space_id);
> -	if (space_id == BOX_ID_NIL || space == NULL) {
> -		sqlite3ErrorMsg(parse, "no such table: %s", tbl_name->zName);
> +	assert(src_name != NULL);
> +	assert(src_name->pTab == NULL);
> +	const char *name = src_name->zName;
> +	struct space *space = space_by_name(name);
> +	if (space == NULL) {
> +		sqlite3ErrorMsg(parse, "no such table: %s", name);

I hate it when one tries to squeeze some unrelated "cleanup", like
this variable renaming, in a patch that doesn't really need it.
This complicates review and clutters the history. I reverted this
part, removed useless assertions, and pushed the patch to 2.1.

>  		return NULL;
>  	}
>  	assert(space != NULL);