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 6547646970E for ; Tue, 24 Dec 2019 03:26:04 +0300 (MSK) Date: Tue, 24 Dec 2019 03:26:02 +0300 From: Nikita Pettik Message-ID: <20191224002602.GA98237@tarantool.org> References: <62bad716-6680-f1b4-f375-15a46b194a5a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <62bad716-6680-f1b4-f375-15a46b194a5a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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");