[Tarantool-patches] [PATCH] sql: extend result set with span
Nikita Pettik
korablev at tarantool.org
Sun Dec 29 23:42:54 MSK 2019
On 29 Dec 19:34, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> I've pushed my review fixes on top of this commit. See it below
> and on the branch. If you agree, then squash. Otherwise lets
> discuss. In my commit you can find inlined explanations about some
> changes.
>
> > diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
> > index 463108001..ca69f4145 100644
> > --- a/test/sql/full_metadata.result
> > +++ b/test/sql/full_metadata.result
> > @@ -56,6 +56,7 @@ execute("SELECT 'aSd' COLLATE \"unicode_ci\";")
> > | ---
> > | - metadata:
> > | - type: string
> > + | span: '''aSd'' COLLATE "unicode_ci"'
> > | name: '''aSd'' COLLATE "unicode_ci"'
> > | collation: unicode_ci
> > | rows:
> > @@ -64,7 +65,8 @@ execute("SELECT 'aSd' COLLATE \"unicode_ci\";")
> > execute("SELECT c FROM t;")
> > | ---
> > | - metadata:
> > - | - type: string
> > + | - span: C
>
> Are you sure this is correct? This is not a pure span. The
> original expression was 'c', but here it is 'C'.
It can be fixed with ease:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index ae7e9fd68..4d69dbc40 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1849,8 +1849,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
if (space->sequence != NULL &&
space->sequence_fieldno == (uint32_t) iCol)
vdbe_metadata_set_col_autoincrement(v, i);
- if (colname != NULL)
- vdbe_metadata_set_col_span(v, i, span);
+ vdbe_metadata_set_col_span(v, i, span);
}
I added this behaviour on purpose. Otherwise, span will be different
from column name and sent as a separate member. Nevetheless, it is
the same column. Probably, we shoudn't return uppercased names in
metadata at all (see https://github.com/tarantool/tarantool/issues/4653).
> ================================================================================
>
> commit 057d42f1784f592d10d5d74303ee6cf7c555dcda
> Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Date: Sun Dec 29 19:30:15 2019 +0300
>
> Review fixes
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 3034cee86..552504365 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -274,6 +274,8 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll,
> {
> uint32_t members_count = 2;
> size_t map_size = 0;
> + if (! sql_metadata_is_full())
> + goto short_meta;
> ================================================================================
>
> Seeing at how fat the full meta has become, I decided it is
> worth skipping it all at once in case we know for sure, that
> it is empty. It will be so in most of the cases.
I've apllied your fixes for select.c, but neglect two of them for
box/lua execute.c where you add goto's. This refactoring doesn't
give any performance gain (speaking of skipping code) but adds
unnecessary gotos making code harder to read and understand
(even to me).
That's what I applied:
diff --git a/src/box/execute.c b/src/box/execute.c
index 3034cee86..c7f5ff24a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -308,10 +308,9 @@ metadata_map_encode(char *buf, const char *name, const char *type,
const char *coll, const char *span, int nullable,
bool is_autoincrement)
{
+ bool is_full = sql_metadata_is_full();
uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1) +
- is_autoincrement;
- if (sql_metadata_is_full())
- map_sz++;
+ is_autoincrement + is_full;
buf = mp_encode_map(buf, map_sz);
buf = mp_encode_uint(buf, IPROTO_FIELD_NAME);
buf = mp_encode_str(buf, name, strlen(name));
@@ -329,21 +328,21 @@ metadata_map_encode(char *buf, const char *name, const char *type,
buf = mp_encode_uint(buf, IPROTO_FIELD_IS_AUTOINCREMENT);
buf = mp_encode_bool(buf, true);
}
- if (sql_metadata_is_full()) {
- /*
- * Span is an original expression that forms
- * result set column. In most cases it is the
- * same as column name. So to avoid sending
- * the same string twice simply encode it as
- * a nil and account this behaviour on client
- * side (see decode_metadata_optional()).
- */
- buf = mp_encode_uint(buf, IPROTO_FIELD_SPAN);
- if (span != NULL)
- buf = mp_encode_str(buf, span, strlen(span));
- else
- buf = mp_encode_nil(buf);
- }
+ if (! is_full)
+ return;
+ /*
+ * Span is an original expression that forms
+ * result set column. In most cases it is the
+ * same as column name. So to avoid sending
+ * the same string twice simply encode it as
+ * a nil and account this behaviour on client
+ * side (see decode_metadata_optional()).
+ */
+ buf = mp_encode_uint(buf, IPROTO_FIELD_SPAN);
+ if (span != NULL)
+ buf = mp_encode_str(buf, span, strlen(span));
+ else
+ buf = mp_encode_nil(buf);
}
/**
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 5ed7b9bd3..8442f610d 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -26,10 +26,9 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
const char *span = sql_column_span(stmt, i);
int nullable = sql_column_nullable(stmt, i);
bool is_autoincrement = sql_column_is_autoincrement(stmt, i);
+ bool is_full = sql_metadata_is_full();
size_t table_sz = 2 + (coll != NULL) + (nullable != -1) +
- is_autoincrement;
- if (sql_metadata_is_full())
- table_sz++;
+ is_autoincrement + is_full;
lua_createtable(L, 0, table_sz);
> ================================================================================
> if (coll != NULL) {
> members_count++;
> map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
> @@ -289,12 +291,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll,
> map_size += mp_sizeof_uint(IPROTO_FIELD_IS_AUTOINCREMENT);
> map_size += mp_sizeof_bool(true);
> }
> - if (sql_metadata_is_full()) {
> - members_count++;
> - map_size += mp_sizeof_uint(IPROTO_FIELD_SPAN);
> - map_size += span != NULL ? mp_sizeof_str(strlen(span)) :
> - mp_sizeof_nil();
> - }
> + members_count++;
> + map_size += mp_sizeof_uint(IPROTO_FIELD_SPAN);
> + map_size += span != NULL ? mp_sizeof_str(strlen(span)) :
> + mp_sizeof_nil();
> +short_meta:
> map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
> map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
> map_size += mp_sizeof_str(strlen(name));
> @@ -308,15 +309,16 @@ metadata_map_encode(char *buf, const char *name, const char *type,
> const char *coll, const char *span, int nullable,
> bool is_autoincrement)
> {
> + bool is_full = sql_metadata_is_full();
> uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1) +
> - is_autoincrement;
> - if (sql_metadata_is_full())
> - map_sz++;
> + is_autoincrement + is_full;
> 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 (! is_full)
> + return;
> if (coll != NULL) {
> buf = mp_encode_uint(buf, IPROTO_FIELD_COLL);
> buf = mp_encode_str(buf, coll, strlen(coll));
> @@ -329,21 +331,18 @@ metadata_map_encode(char *buf, const char *name, const char *type,
> buf = mp_encode_uint(buf, IPROTO_FIELD_IS_AUTOINCREMENT);
> buf = mp_encode_bool(buf, true);
> }
> - if (sql_metadata_is_full()) {
> - /*
> - * Span is an original expression that forms
> - * result set column. In most cases it is the
> - * same as column name. So to avoid sending
> - * the same string twice simply encode it as
> - * a nil and account this behaviour on client
> - * side (see decode_metadata_optional()).
> - */
> - buf = mp_encode_uint(buf, IPROTO_FIELD_SPAN);
> - if (span != NULL)
> - buf = mp_encode_str(buf, span, strlen(span));
> - else
> - buf = mp_encode_nil(buf);
> - }
> + /*
> + * Span is an original expression that forms result set
> + * column. In most cases it is the same as column name. So
> + * to avoid sending the same string twice simply encode it
> + * as a nil and account this behaviour on client side (see
> + * decode_metadata_optional()).
> + */
> + buf = mp_encode_uint(buf, IPROTO_FIELD_SPAN);
> + if (span != NULL)
> + buf = mp_encode_str(buf, span, strlen(span));
> + else
> + buf = mp_encode_nil(buf);
> }
>
> /**
> diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
> index 5ed7b9bd3..d019e69e2 100644
> --- a/src/box/lua/execute.c
> +++ b/src/box/lua/execute.c
> @@ -26,10 +26,9 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
> const char *span = sql_column_span(stmt, i);
> int nullable = sql_column_nullable(stmt, i);
> bool is_autoincrement = sql_column_is_autoincrement(stmt, i);
> + bool is_full = sql_metadata_is_full();
> size_t table_sz = 2 + (coll != NULL) + (nullable != -1) +
> - is_autoincrement;
> - if (sql_metadata_is_full())
> - table_sz++;
> + is_autoincrement + is_full;
> lua_createtable(L, 0, table_sz);
> /*
> * Can not fail, since all column names are
> @@ -42,6 +41,8 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
> lua_setfield(L, -2, "name");
> lua_pushstring(L, type);
> lua_setfield(L, -2, "type");
> + if (!is_full)
> + goto set_column;
> if (coll != NULL) {
> lua_pushstring(L, coll);
> lua_setfield(L, -2, "collation");
> @@ -54,13 +55,12 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
> lua_pushboolean(L, true);
> lua_setfield(L, -2, "is_autoincrement");
> }
> - if (sql_metadata_is_full()) {
> - if (span != NULL)
> - lua_pushstring(L, span);
> - else
> - lua_pushstring(L, name);
> - lua_setfield(L, -2, "span");
> - }
> + if (span != NULL)
> + lua_pushstring(L, span);
> + else
> + lua_pushstring(L, name);
> + lua_setfield(L, -2, "span");
> + set_column:
> lua_rawseti(L, -2, i + 1);
> }
> }
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index b4182b88a..ae7e9fd68 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1814,6 +1814,8 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> }
> }
> vdbe_metadata_set_col_nullability(v, i, -1);
> + const char *colname = pEList->a[i].zName;
> + const char *span = pEList->a[i].zSpan;
> if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
> char *zCol;
> int iCol = p->iColumn;
> @@ -1826,12 +1828,10 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> struct space_def *space_def = space->def;
> assert(iCol >= 0 && iCol < (int)space_def->field_count);
> zCol = space_def->fields[iCol].name;
> - const char *name = NULL;
> - if (pEList->a[i].zName != NULL) {
> - name = pEList->a[i].zName;
> - } else {
> + const char *name = colname;
> + if (name == NULL) {
> if (!shortNames && !fullNames) {
> - name = pEList->a[i].zSpan;
> + name = span;
> } else if (fullNames) {
> name = tt_sprintf("%s.%s",
> space_def->name,
> @@ -1849,26 +1849,20 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> if (space->sequence != NULL &&
> space->sequence_fieldno == (uint32_t) iCol)
> vdbe_metadata_set_col_autoincrement(v, i);
> - if (pEList->a[i].zName != NULL) {
> - vdbe_metadata_set_col_span(v, i,
> - pEList->a[i].zSpan);
> - }
> + if (colname != NULL)
> + vdbe_metadata_set_col_span(v, i, span);
> }
> } else {
> const char *z = NULL;
> - if (pEList->a[i].zName != NULL)
> - z = pEList->a[i].zName;
> - else if (pEList->a[i].zSpan != NULL)
> - z = pEList->a[i].zSpan;
> + if (colname != NULL)
> + z = colname;
> + else if (span != NULL)
> + z = span;
> else
> z = tt_sprintf("column%d", i + 1);
> vdbe_metadata_set_col_name(v, i, z);
> - if (is_full_meta) {
> - if (pEList->a[i].zName != NULL) {
> - vdbe_metadata_set_col_span(v, i,
> - pEList->a[i].zSpan);
> - }
> - }
> + if (is_full_meta && colname != NULL)
> + vdbe_metadata_set_col_span(v, i, span);
> }
> }
> if (var_count == 0)
More information about the Tarantool-patches
mailing list