[Tarantool-patches] [PATCH 4/6] sql: extend result set with nullability
Nikita Pettik
korablev at tarantool.org
Thu Dec 5 14:50:51 MSK 2019
On 28 Nov 23:41, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> You added a leading whitespace to the commit title.
Fixed.
> Here is a strange example:
>
> box.cfg{}
> s = box.schema.create_space('TEST', {
> format = {{'ID', 'unsigned'}, {'COL', 'unsigned', is_nullable = true}}
> })
> pk = s:create_index('pk', {parts = {{'ID'}, {'COL', is_nullable = false}}})
>
> In that example I defined a 'false-nullable' column. It
> is nullable in the space format, but is not in the
> index format. The strictest rule wins, so it is not nullable
> after all. That can be seen in struct tuple_format. SQL says,
> that it is nullable. But this is not a real problem. Perhaps
> this might be even right.
If it was up to me, I would disallow such possible inconsistency
between space and index formats. It turns out to be quite
confusing. I believe there's historical reason for that, tho.
>However the next example certainly
> misses something:
>
> box.execute('SELECT * FROM test')
> ---
> - metadata:
> - name: ID
> type: unsigned
> - name: COL
> type: unsigned
> rows: []
> ...
>
> box.execute('SELECT col FROM test')
> ---
> - metadata:
> - type: unsigned
> name: COL
> is_nullable: true
> rows: []
> ...
>
> As you can see, nullable depends on whether I select a
> column explicitly or not. Perhaps the same happens to
> autoincrement meta, I didn't check.
I'm glad you found this bug, thanks. To fix that I have to
add a bit more refactoring which I've introduced in the
first patch:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d7975bc7f..e4768121e 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1794,10 +1794,7 @@ 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 (pEList->a[i].zName) {
- char *zName = pEList->a[i].zName;
- vdbe_metadata_set_col_name(v, i, zName);
- } else if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
+ if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
for (j = 0; ALWAYS(j < pTabList->nSrc); j++) {
@@ -1808,20 +1805,28 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
struct space_def *space_def = pTabList->a[j].space->def;
assert(iCol >= 0 && iCol < (int)space_def->field_count);
zCol = space_def->fields[iCol].name;
- if (!shortNames && !fullNames) {
- vdbe_metadata_set_col_name(v, i,
- pEList->a[i].zSpan);
- } else if (fullNames) {
- const char *zName = tt_sprintf("%s.%s",
- space_def->name,
- zCol);
- vdbe_metadata_set_col_name(v, i, zName);
+ const char *name = NULL;
+ if (pEList->a[i].zName != NULL) {
+ name = pEList->a[i].zName;
} else {
- vdbe_metadata_set_col_name(v, i, zCol);
+ if (!shortNames && !fullNames) {
+ name = pEList->a[i].zSpan;
+ } else if (fullNames) {
+ name = tt_sprintf("%s.%s",
+ space_def->name,
+ zCol);
+ } else {
+ name = zCol;
+ }
}
+ vdbe_metadata_set_col_name(v, i, name);
} else {
- const char *z = pEList->a[i].zSpan;
- if (z == NULL)
+ 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;
+ else
z = tt_sprintf("column%d", i + 1);
vdbe_metadata_set_col_name(v, i, z);
}
> > diff --git a/src/box/execute.c b/src/box/execute.c
> > index 20bfd0957..98812ae1e 100644
> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -267,8 +267,10 @@ error:
> > region_truncate(region, svp);
> > return -1;
> > }
> > +
> > static size_t
> > -metadata_map_sizeof(const char *name, const char *type, const char *coll)
> > +metadata_map_sizeof(const char *name, const char *type, const char *coll,
> > + int nullable)
>
> 1. Please, rename to 'is_nullable'. Here and in other places.
IMHO 'is_' prefix implies boolean value, meanwhile here nullable
is in fact three-valued: 0, 1 implies nullable/not nullable, -1
is unknown (for columns of resulting set that are complex expressions).
So that we can avoid sending 'nullable' property in meta for expressions,
but always set it for columns. Hence, I omitted is_ prefix on purpose.
> > {
> > uint32_t members_count = 2;
> > size_t map_size = 0;
> > @@ -277,6 +279,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll)
> > map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
> > map_size += mp_sizeof_str(strlen(coll));
> > }
> > + if (nullable != -1) {
> > + members_count++;
> > + map_size += mp_sizeof_uint(IPROTO_FIELD_NULLABLE);
>
> 2. This too. IPROTO_FIELD_IS_NULLABLE.
Ok, here it makes sense:
diff --git a/src/box/execute.c b/src/box/execute.c
index 98812ae1e..0616638ab 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -281,7 +281,7 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll,
}
if (nullable != -1) {
members_count++;
- map_size += mp_sizeof_uint(IPROTO_FIELD_NULLABLE);
+ map_size += mp_sizeof_uint(IPROTO_FIELD_IS_NULLABLE);
map_size += mp_sizeof_bool(nullable);
}
map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
@@ -343,7 +343,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
pos = mp_encode_str(pos, coll, strlen(coll));
}
if (nullable != -1) {
- pos = mp_encode_uint(pos, IPROTO_FIELD_NULLABLE);
+ pos = mp_encode_uint(pos, IPROTO_FIELD_IS_NULLABLE);
pos = mp_encode_bool(pos, nullable);
}
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 030c25531..53d014b60 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -132,7 +132,7 @@ enum iproto_metadata_key {
IPROTO_FIELD_NAME = 0,
IPROTO_FIELD_TYPE = 1,
IPROTO_FIELD_COLL = 2,
- IPROTO_FIELD_NULLABLE = 3
+ IPROTO_FIELD_IS_NULLABLE = 3,
};
enum iproto_ballot_key {
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 3e93cbc75..9dfcebd23 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -652,7 +652,7 @@ decode_metadata_optional(struct lua_State *L, const char **data,
lua_pushlstring(L, coll, len);
lua_setfield(L, -2, "collation");
} else {
- assert(key == IPROTO_FIELD_NULLABLE);
+ assert(key == IPROTO_FIELD_IS_NULLABLE);
bool is_nullable = mp_decode_bool(data);
lua_pushboolean(L, is_nullable);
lua_setfield(L, -2, "is_nullable");
> > @@ -334,6 +342,10 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
> > pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
> > pos = mp_encode_str(pos, coll, strlen(coll));
> > }
> > + if (nullable != -1) {
> > + pos = mp_encode_uint(pos, IPROTO_FIELD_NULLABLE);
> > + pos = mp_encode_bool(pos, nullable);
> > + }
>
> 3. We send autoincrement only in case it is true.
> Then how about sending nullability only in case it
> is true? To save a bit of the network.
Autoincrement can appear only once in the result set, so it barely
makes sense to send it for each column. On the other hand, we tell
column's false nullability from 'unknown' nullability of expressions.
That's what I want to underline using three-valued nullability status
in meta.
> > }
> > return 0;
> > }
> > diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
> > index fa9c029a2..030c25531 100644
> > --- a/src/box/iproto_constants.h
> > +++ b/src/box/iproto_constants.h
> > @@ -132,6 +132,7 @@ enum iproto_metadata_key {
> > IPROTO_FIELD_NAME = 0,
> > IPROTO_FIELD_TYPE = 1,
> > IPROTO_FIELD_COLL = 2,
> > + IPROTO_FIELD_NULLABLE = 3
>
> 4. Please, add a comma after 3 so as not to
> change this line in the next commit.
Fixed.
> > index 66e8c1274..b772bcead 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1836,7 +1837,10 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> > } else {
> > vdbe_set_metadata_col_name(v, i, zCol);
> > }
> > -
> > + bool is_nullable = space_def->fields[iCol].is_nullable;
> > + if (p->op == TK_COLUMN)
> > + vdbe_set_metadata_col_nullability(v, i,
> > + is_nullable);
>
> 5. Please, wrap into {}.
Skipped (taking into consideration refactoring at the beginning of letter).
New patch (I exluded test changes from diff):
Author: Nikita Pettik <korablev at tarantool.org>
Date: Mon Nov 25 23:04:58 2019 +0300
sql: extend result set with nullability
If member of result set is (solely) column identifier, then metadata
will contain its corresponding field nullability as boolean property.
Note that indicating nullability for other expressions (like x + 1)
may make sense but it requires derived nullability calculation which in
turn seems to be overkill (at least in scope of current patch).
Part of #4407
diff --git a/src/box/execute.c b/src/box/execute.c
index 72300235a..0616638ab 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -269,7 +269,8 @@ error:
}
static size_t
-metadata_map_sizeof(const char *name, const char *type, const char *coll)
+metadata_map_sizeof(const char *name, const char *type, const char *coll,
+ int nullable)
{
uint32_t members_count = 2;
size_t map_size = 0;
@@ -278,6 +279,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll)
map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
map_size += mp_sizeof_str(strlen(coll));
}
+ if (nullable != -1) {
+ members_count++;
+ map_size += mp_sizeof_uint(IPROTO_FIELD_IS_NULLABLE);
+ map_size += mp_sizeof_bool(nullable);
+ }
map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
map_size += mp_sizeof_str(strlen(name));
@@ -312,6 +318,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
+ int nullable = sql_column_is_nullable(stmt, i);
/*
* Can not fail, since all column names and types
* are preallocated during prepare phase and the
@@ -319,13 +326,13 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
*/
assert(name != NULL);
assert(type != NULL);
- size = metadata_map_sizeof(name, type, coll);
+ size = metadata_map_sizeof(name, type, coll, nullable);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
return -1;
}
- uint32_t map_sz = 2 + (coll != NULL);
+ uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1);
pos = mp_encode_map(pos, map_sz);
pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
pos = mp_encode_str(pos, name, strlen(name));
@@ -335,6 +342,10 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
pos = mp_encode_str(pos, coll, strlen(coll));
}
+ if (nullable != -1) {
+ pos = mp_encode_uint(pos, IPROTO_FIELD_IS_NULLABLE);
+ pos = mp_encode_bool(pos, nullable);
+ }
}
return 0;
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index fa9c029a2..53d014b60 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -132,6 +132,7 @@ enum iproto_metadata_key {
IPROTO_FIELD_NAME = 0,
IPROTO_FIELD_TYPE = 1,
IPROTO_FIELD_COLL = 2,
+ IPROTO_FIELD_IS_NULLABLE = 3,
};
enum iproto_ballot_key {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 8a530bfc1..3d7ca710c 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -24,6 +24,7 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
lua_createtable(L, 0, coll != NULL ? 3 : 2);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
+ int nullable = sql_column_is_nullable(stmt, i);
/*
* Can not fail, since all column names are
* preallocated during prepare phase and the
@@ -39,6 +40,10 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
lua_pushstring(L, coll);
lua_setfield(L, -2, "collation");
}
+ if (nullable != -1) {
+ lua_pushboolean(L, nullable);
+ lua_setfield(L, -2, "is_nullable");
+ }
lua_rawseti(L, -2, i + 1);
}
}
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index afbd1e1be..9dfcebd23 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -651,6 +651,11 @@ decode_metadata_optional(struct lua_State *L, const char **data,
const char *coll = mp_decode_str(data, &len);
lua_pushlstring(L, coll, len);
lua_setfield(L, -2, "collation");
+ } else {
+ assert(key == IPROTO_FIELD_IS_NULLABLE);
+ bool is_nullable = mp_decode_bool(data);
+ lua_pushboolean(L, is_nullable);
+ lua_setfield(L, -2, "is_nullable");
}
}
}
@@ -667,7 +672,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
lua_createtable(L, count, 0);
for (uint32_t i = 0; i < count; ++i) {
uint32_t map_size = mp_decode_map(data);
- assert(map_size == 2 || map_size == 3);
+ assert(map_size >= 2 && map_size <= 4);
(void) map_size;
uint32_t key = mp_decode_uint(data);
assert(key == IPROTO_FIELD_NAME);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index c0a1d244c..a0ffbb670 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1838,6 +1838,8 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
}
}
vdbe_metadata_set_col_name(v, i, name);
+ bool is_nullable = space_def->fields[iCol].is_nullable;
+ vdbe_metadata_set_col_nullability(v, i, is_nullable);
} else {
const char *z = NULL;
if (pEList->a[i].zName != NULL)
@@ -1847,6 +1849,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
else
z = tt_sprintf("column%d", i + 1);
vdbe_metadata_set_col_name(v, i, z);
+ vdbe_metadata_set_col_nullability(v, i, -1);
}
}
if (var_count == 0)
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4c2e3ed73..89920b3d1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -579,6 +579,9 @@ sql_column_datatype(sql_stmt *, int N);
const char *
sql_column_coll(sql_stmt *stmt, int n);
+int
+sql_column_is_nullable(sql_stmt *stmt, int n);
+
int
sql_initialize(void);
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 22ba0b756..384d7dc0a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -257,6 +257,9 @@ int
vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
size_t coll_len);
+void
+vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable);
+
void sqlVdbeCountChanges(Vdbe *);
sql *sqlVdbeDb(Vdbe *);
void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index d3de5770b..92a50dd7b 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -350,6 +350,11 @@ struct sql_column_metadata {
char *name;
char *type;
char *collation;
+ /**
+ * -1 is for any member of result set except for pure
+ * columns: all other expressions are nullable by default.
+ */
+ int8_t nullable : 2;
};
/*
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index e57a80334..1d6971223 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -753,6 +753,13 @@ sql_column_coll(sql_stmt *stmt, int n)
return p->metadata[n].collation;
}
+int
+sql_column_is_nullable(sql_stmt *stmt, int n)
+{
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].nullable;
+}
/******************************* sql_bind_ **************************
*
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 25148a49a..6b467951b 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1904,6 +1904,13 @@ vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
return 0;
}
+void
+vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable)
+{
+ assert(idx < p->nResColumn);
+ p->metadata[idx].nullable = nullable;
+}
+
/*
* This routine checks that the sql.nVdbeActive count variable
* matches the number of vdbe's in the list sql.pVdbe that are
More information about the Tarantool-patches
mailing list