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

[Nikita Pettik <korablev@tarantool.org>]

On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 3 comments below.
> 
> On 11/12/2019 14:44, 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.
> > 
> > This patch also introduces new pragma: full_metadata. By default it is
> > not set. If it is turned on, then optional metadata (like collation) is
> > pushed to Lua stack. Note that via IProto protocol always full metadata
> > is send, but its decoding depends on session SQL settings.
> > 
> > Part of #4407
> > diff --git a/src/box/execute.c b/src/box/execute.c
> > index e8b012e5b..72300235a 100644
> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -268,6 +268,24 @@ error:
> >  	return -1;
> >  }
> >  
> > +static size_t
> > +metadata_map_sizeof(const char *name, const char *type, const char *coll)
> 
> 1. Since you moved sizeof to a separate function, maybe it is
> worth doing the same with encoding part.
> 
> > +{
> > +	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/sql/select.c b/src/box/sql/select.c
> > index e4768121e..506c1a7c7 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1794,6 +1795,24 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> >  			var_pos[var_count++] = i;
> >  		vdbe_metadata_set_col_type(v, i,
> >  					   field_type_strs[sql_expr_type(p)]);
> > +		if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
> > +			bool unused;
> > +			uint32_t id;
> 
> 2. I would initialize id with 0, because a compiler may complain
> about usage of not initialized id below. Now it does not, but
> it can start, after any change in the code or in the compiler
> version.
> 
> > +			struct coll *coll = NULL;
> > +			/*
> > +			 * If sql_expr_coll fails then it fails somewhere
> > +			 * above the call stack.
> > +			 */
> > +			int rc =  sql_expr_coll(pParse, p, &unused, &id, &coll);
> > +			assert(rc == 0);
> > +			(void) rc;
> > +			if (id != COLL_NONE) {
> > +				struct coll_id *coll_id = coll_by_id(id);
> > +				vdbe_metadata_set_col_collation(v, i,
> > +								coll_id->name,
> > +								coll_id->name_len);
> > +			}
> > +		}
> >  		if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
> >  			char *zCol;
> >  			int iCol = p->iColumn;
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index f2cf386bb..02d9fc6ce 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1888,6 +1889,21 @@ vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
> >  	return 0;
> >  }
> >  
> > +int
> > +vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
> > +				size_t coll_len)
> > +{
> > +	assert(idx < p->nResColumn);
> > +	if (p->metadata[idx].collation != NULL)
> > +		free((void *)p->metadata[idx].collation);
> 
> 3. Please, remove (void *) cast. You don't need it since you
> dropped 'const' from sql_column_metadata members. In the next
> patches in similar places too.
> 
> > +	p->metadata[idx].collation = strndup(coll, coll_len);
> > +	if (p->metadata[idx].collation == NULL) {
> > +		diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");
> > +		return -1;
> > +	}
> > +	return 0;
> > +}

Fixed all nits:

diff --git a/src/box/execute.c b/src/box/execute.c
index 72300235a..887e05eb8 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -286,6 +286,22 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll)
        return map_size;
 }
 
+static inline void
+metadata_map_encode(char *buf, const char *name, const char *type,
+                   const char *coll)
+{
+       uint32_t map_sz = 2 + (coll != NULL);
+       buf = mp_encode_map(buf, map_sz);
+       buf = mp_encode_uint(buf, IPROTO_FIELD_NAME);
+       buf = mp_encode_str(buf, name, strlen(name));
+       buf = mp_encode_uint(buf, IPROTO_FIELD_TYPE);
+       buf = mp_encode_str(buf, type, strlen(type));
+       if (coll != NULL) {
+               buf = mp_encode_uint(buf, IPROTO_FIELD_COLL);
+               buf = mp_encode_str(buf, coll, strlen(coll));
+       }
+}
+
 /**
  * Serialize a description of the prepared statement.
  * @param stmt Prepared statement.
@@ -325,16 +341,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
                        diag_set(OutOfMemory, size, "obuf_alloc", "pos");
                        return -1;
                }
-               uint32_t map_sz = 2 + (coll != NULL);
-               pos = mp_encode_map(pos, map_sz);
-               pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
-               pos = mp_encode_str(pos, name, strlen(name));
-               pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
-               pos = mp_encode_str(pos, type, strlen(type));
-               if (coll != NULL) {
-                       pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
-                       pos = mp_encode_str(pos, coll, strlen(coll));
-               }
+               metadata_map_encode(pos, name, type, coll);
        }
        return 0;
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 506c1a7c7..a23d2692e 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1797,7 +1797,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
                                           field_type_strs[sql_expr_type(p)]);
                if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
                        bool unused;
-                       uint32_t id;
+                       uint32_t id = 0;
                        struct coll *coll = NULL;
                        /*
                         * If sql_expr_coll fails then it fails somewhere
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 02d9fc6ce..bc9aed81a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1895,7 +1895,7 @@ vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
 {
        assert(idx < p->nResColumn);
        if (p->metadata[idx].collation != NULL)
-               free((void *)p->metadata[idx].collation);
+               free(p->metadata[idx].collation);
        p->metadata[idx].collation = strndup(coll, coll_len);
        if (p->metadata[idx].collation == NULL) {
                diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");