From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 8989F46970E for ; Tue, 24 Dec 2019 03:44:33 +0300 (MSK) Date: Tue, 24 Dec 2019 03:44:32 +0300 From: Nikita Pettik Message-ID: <20191224004432.GB41539@tarantool.org> References: <05f3c08a8639c188ffe1e7459d0d14b8bad3dd86.1574846892.git.korablev@tarantool.org> <20191218110829.GA28206@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191218110829.GA28206@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: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org On 18 Dec 14:08, Sergey Ostanevich wrote: > 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. Ok, added inline attribute. I've pushed fresh branch, so you can chech changes. > > +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. Other cases (nullability, autoincrement, alias) are added in further patches. > 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? When we decode map containing metadata, we check its size. So we can't decode more than given number of map members. In this particular patch, metadata can consist of 2 (name and type are required) or 3 (+ collation which is optional) members. There should be assertion verifying that key is always IPROTO_FIELD_COLL, but to avoid diffs from patch to patch, I added assertion only in the last patch. > > +/** 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