Re: [Tarantool-patches] [PATCH 3/6] sql: extend result set with collation

[Nikita Pettik <korablev@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