Re: [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 3 comments below.

On 11/12/2019 14:44, Nikita Pettik wrote:
> If resulting set column is of STRING type and features collation (no
> matter explicit or implicit) different from "none", then metadata will
> contain its name.
> 
> This patch also introduces new pragma: full_metadata. By default it is
> not set. If it is turned on, then optional metadata (like collation) is
> pushed to Lua stack. Note that via IProto protocol always full metadata
> is send, but its decoding depends on session SQL settings.
> 
> Part of #4407
> diff --git a/src/box/execute.c b/src/box/execute.c
> index e8b012e5b..72300235a 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -268,6 +268,24 @@ error:
>  	return -1;
>  }
>  
> +static size_t
> +metadata_map_sizeof(const char *name, const char *type, const char *coll)

1. Since you moved sizeof to a separate function, maybe it is
worth doing the same with encoding part.

> +{
> +	uint32_t members_count = 2;
> +	size_t map_size = 0;
> +	if (coll != NULL) {
> +		members_count++;
> +		map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
> +		map_size += mp_sizeof_str(strlen(coll));
> +	}
> +	map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
> +	map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
> +	map_size += mp_sizeof_str(strlen(name));
> +	map_size += mp_sizeof_str(strlen(type));
> +	map_size += mp_sizeof_map(members_count);
> +	return map_size;
> +}
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index e4768121e..506c1a7c7 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1794,6 +1795,24 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  			var_pos[var_count++] = i;
>  		vdbe_metadata_set_col_type(v, i,
>  					   field_type_strs[sql_expr_type(p)]);
> +		if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
> +			bool unused;
> +			uint32_t id;

2. I would initialize id with 0, because a compiler may complain
about usage of not initialized id below. Now it does not, but
it can start, after any change in the code or in the compiler
version.

> +			struct coll *coll = NULL;
> +			/*
> +			 * If sql_expr_coll fails then it fails somewhere
> +			 * above the call stack.
> +			 */
> +			int rc =  sql_expr_coll(pParse, p, &unused, &id, &coll);
> +			assert(rc == 0);
> +			(void) rc;
> +			if (id != COLL_NONE) {
> +				struct coll_id *coll_id = coll_by_id(id);
> +				vdbe_metadata_set_col_collation(v, i,
> +								coll_id->name,
> +								coll_id->name_len);
> +			}
> +		}
>  		if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
>  			char *zCol;
>  			int iCol = p->iColumn;
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index f2cf386bb..02d9fc6ce 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1888,6 +1889,21 @@ vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
>  	return 0;
>  }
>  
> +int
> +vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
> +				size_t coll_len)
> +{
> +	assert(idx < p->nResColumn);
> +	if (p->metadata[idx].collation != NULL)
> +		free((void *)p->metadata[idx].collation);

3. Please, remove (void *) cast. You don't need it since you
dropped 'const' from sql_column_metadata members. In the next
patches in similar places too.

> +	p->metadata[idx].collation = strndup(coll, coll_len);
> +	if (p->metadata[idx].collation == NULL) {
> +		diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");
> +		return -1;
> +	}
> +	return 0;
> +}