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

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch!
I'm looking into the latest version of the changes in the branch - just
two comments below.

> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -267,6 +267,23 @@ error:
>  	region_truncate(region, svp);
>  	return -1;
>  }
> +static size_t

Would you consider put inline here, since it has only one call site. It
also alinged with other static function definitions in this file.

> +metadata_map_sizeof(const char *name, const char *type, const char *coll)
> +{
...
> 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;
>  }
>  

The naming and 'while' logic implies that you plan to support more than
just 'collation' case. While in the code you have no actions for
different types of metadata. Should it skip next string from the data 
after key is not IPROTO_FIELD_COLL at least - otherwise we will try to
decode next int from string data?

> +/** 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");
> +		}
> +	}
> +}
> +
>  /**
>   * Decode IPROTO_METADATA into array of maps.
>   * @param L Lua stack to push result on.


regards,
Sergos