[Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Dec 18 03:29:48 MSK 2019
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;
> +}
More information about the Tarantool-patches
mailing list