From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 1E82D46970E for ; Wed, 18 Dec 2019 14:08:31 +0300 (MSK) Date: Wed, 18 Dec 2019 14:08:29 +0300 From: Sergey Ostanevich Message-ID: <20191218110829.GA28206@tarantool.org> References: <05f3c08a8639c188ffe1e7459d0d14b8bad3dd86.1574846892.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <05f3c08a8639c188ffe1e7459d0d14b8bad3dd86.1574846892.git.korablev@tarantool.org> 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 Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@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