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 872834696C1 for ; Fri, 29 Nov 2019 01:41:56 +0300 (MSK) References: <05f3c08a8639c188ffe1e7459d0d14b8bad3dd86.1574846892.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <089dc655-bba0-ecf1-db00-3ff8c2729575@tarantool.org> Date: Thu, 28 Nov 2019 23:41:53 +0100 MIME-Version: 1.0 In-Reply-To: <05f3c08a8639c188ffe1e7459d0d14b8bad3dd86.1574846892.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 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 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)