From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A74294696C4 for ; Wed, 18 Dec 2019 03:29:50 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <62bad716-6680-f1b4-f375-15a46b194a5a@tarantool.org> Date: Wed, 18 Dec 2019 01:29:48 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.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; > +}