[Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation
Nikita Pettik
korablev at tarantool.org
Tue Dec 24 03:26:02 MSK 2019
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");
More information about the Tarantool-patches
mailing list