Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: Sergey Ostanevich <sergos@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 3/6] sql: extend result set with collation
Date: Tue, 24 Dec 2019 03:44:32 +0300	[thread overview]
Message-ID: <20191224004432.GB41539@tarantool.org> (raw)
In-Reply-To: <20191218110829.GA28206@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

  reply	other threads:[~2019-12-24  0:44 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-27 12:15 [Tarantool-patches] [PATCH 0/6] sql: extend response metadata Nikita Pettik
2019-11-27 12:15 ` [Tarantool-patches] [PATCH 1/6] sql: refactor resulting set metadata Nikita Pettik
2019-11-28 22:41   ` Vladislav Shpilevoy
2019-12-05 11:39     ` Nikita Pettik
2019-12-05 23:58       ` Vladislav Shpilevoy
2019-12-06 12:48         ` Nikita Pettik
2019-12-17 13:23   ` Sergey Ostanevich
2019-11-27 12:15 ` [Tarantool-patches] [PATCH 2/6] sql: fix possible null dereference in sql_expr_coll() Nikita Pettik
2019-11-28 22:42   ` Vladislav Shpilevoy
2019-12-05 11:40     ` Nikita Pettik
2019-12-05 23:59       ` Vladislav Shpilevoy
2019-12-06 12:48         ` Nikita Pettik
2019-12-17 13:30           ` Sergey Ostanevich
2019-12-17 14:44             ` Nikita Pettik
2019-12-17 19:53               ` Nikita Pettik
2019-11-27 12:15 ` [Tarantool-patches] [PATCH 3/6] sql: extend result set with collation Nikita Pettik
2019-11-28 22:41   ` Vladislav Shpilevoy
2019-12-05 11:50     ` Nikita Pettik
2019-12-18 11:08   ` Sergey Ostanevich
2019-12-24  0:44     ` Nikita Pettik [this message]
2019-11-27 12:15 ` [Tarantool-patches] [PATCH 4/6] sql: extend result set with nullability Nikita Pettik
2019-11-28 22:41   ` Vladislav Shpilevoy
2019-12-05 11:50     ` Nikita Pettik
2019-12-06  0:00       ` Vladislav Shpilevoy
2019-12-06 12:49         ` Nikita Pettik
2019-12-18 13:31   ` Sergey Ostanevich
2019-11-27 12:15 ` [Tarantool-patches] [PATCH 5/6] sql: extend result set with autoincrement Nikita Pettik
2019-11-28 22:41   ` Vladislav Shpilevoy
2019-12-05 11:51     ` Nikita Pettik
2019-12-18 15:17   ` Sergey Ostanevich
2019-12-24  0:47     ` Nikita Pettik
2019-11-27 12:15 ` [Tarantool-patches] [PATCH 6/6] sql: extend result set with alias Nikita Pettik
2019-11-28 22:41   ` Vladislav Shpilevoy
2019-12-05 11:51     ` Nikita Pettik
2019-12-06  0:02       ` Vladislav Shpilevoy
2019-12-06 12:50         ` Nikita Pettik
2019-12-06 21:52           ` Vladislav Shpilevoy
2019-12-19 15:17   ` Sergey Ostanevich
2019-12-24  0:27     ` Nikita Pettik
2019-11-28 22:55 ` [Tarantool-patches] [PATCH 0/6] sql: extend response metadata Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191224004432.GB41539@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 3/6] sql: extend result set with collation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox