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

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

Thanks for the patch!

I see, that we calculate nullable and autoinc for
table columns only. And it makes sense. Then maybe
we need collations and alias also for columns only,
not for all strings and result set columns? Could you
please investigate? I think Alexander knows.

See 4 comments below.

On 27/11/2019 13:15, 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.
> 
> Part of #4407
> ---
>  src/box/execute.c          |  31 +++++++--
>  src/box/iproto_constants.h |   1 +
>  src/box/lua/execute.c      |   7 +-
>  src/box/lua/net_box.c      |  20 +++++-
>  src/box/sql/select.c       |  18 ++++++
>  src/box/sql/sqlInt.h       |   3 +
>  src/box/sql/vdbe.h         |   4 ++
>  src/box/sql/vdbeInt.h      |   1 +
>  src/box/sql/vdbeapi.c      |   9 +++
>  src/box/sql/vdbeaux.c      |  16 +++++
>  test/sql/collation.result  | 155 +++++++++++++++++++++++++++------------------
>  11 files changed, 195 insertions(+), 70 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index e8b012e5b..20bfd0957 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -267,6 +267,23 @@ error:
>  	region_truncate(region, svp);
>  	return -1;
>  }
> +static size_t

1. Please, add an empty line between these 2 functions.
(In this commit.)

> +metadata_map_sizeof(const char *name, const char *type, const char *coll)
> +{
> +	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/lua/net_box.c b/src/box/lua/net_box.c
> index 001af95dc..afbd1e1be 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -638,6 +638,23 @@ netbox_decode_select(struct lua_State *L)
>  	return 2;
>  }
>  
> +/** Decode optional (i.e. may be present in response) metadata fields. */
> +static void
> +decode_metadata_optional(struct lua_State *L, const char **data,
> +			 uint32_t map_size)
> +{
> +	/* 2 is default metadata map size (field name + field size). */
> +	while (map_size-- > 2) {
> +		uint32_t key = mp_decode_uint(data);
> +		uint32_t len;
> +		if (key == IPROTO_FIELD_COLL) {
> +			const char *coll = mp_decode_str(data, &len);
> +			lua_pushlstring(L, coll, len);
> +			lua_setfield(L, -2, "collation");
> +		}
> +	}

2. Netbox relies on certain order of fields in iproto
response so as to avoid cycles and checking for already
decoded fields. You can safely use it too. To eliminate
the 'while' cycle you can append to netbox_decode_metadata's
cycle:

    if (map_size == 2)
        continue;
    key = mp_decode_uint(data);
    if (key == IPROTO_FIELD_COLL) {
        /* handle coll ... */
    }
    if (map_size == 3)
        continue;
    key = mp_decode_uint(data);
    if (key == IPROTO_FIELD_NULLABLE) {
        /* handle nullable ... */
    }

> +}
> +
>  /**
>   * Decode IPROTO_METADATA into array of maps.
>   * @param L Lua stack to push result on.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index d6b8a158f..66e8c1274 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1794,6 +1794,22 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  			var_pos[var_count++] = i;
>  		vdbe_set_metadata_col_type(v, i,
>  					   field_type_strs[sql_expr_type(p)]);
> +		if (sql_expr_type(p) == FIELD_TYPE_STRING) {
> +			bool unused;
> +			uint32_t id;
> +			struct coll *coll = NULL;
> +			/*
> +			 * If sql_expr_coll fails then it fails somewhere
> +			 * above the call stack.
> +			 */

3. And lets add an assertion for that.

> +			(void) sql_expr_coll(pParse, p, &unused, &id, &coll);
> +			if (id != COLL_NONE) {
> +				struct coll_id *coll_id = coll_by_id(id);
> +				vdbe_set_metadata_col_collation(v, i,
> +								coll_id->name,
> +								coll_id->name_len);
> +			}
> +		}
>  		if (pEList->a[i].zName) {
>  			char *zName = pEList->a[i].zName;
>  			vdbe_set_metadata_col_name(v, i, zName);
> @@ -1819,6 +1836,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  			} else {
>  				vdbe_set_metadata_col_name(v, i, zCol);
>  			}
> +
>  		} else {

4. How about drop of these two hunks consisting of empty lines?

>  			const char *z = pEList->a[i].zSpan;
>  			if (z == NULL)