[Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Dec 18 03:29:43 MSK 2019
Thanks for the patch!
See 2 comments below.
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 169814a2e..cc5891bb8 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -420,6 +420,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> sqlVdbeSetNumCols(v, 1);
> vdbe_metadata_set_col_name(v, 0, "rows deleted");
> vdbe_metadata_set_col_type(v, 0, "integer");
> + vdbe_metadata_set_col_nullability(v, 0, -1);
1. All these set_col_nullability(-1) look really doubtful. You
don't set collation NULL explicitly, but you set nullability -1
explicitly. Why?
I think it is much better to fill default values of newly allocated
sql_column_metadata in sqlVdbeSetNumCols. See my diff (not on the
branch):
==================================================================
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index cc5891bb8..169814a2e 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -420,7 +420,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlVdbeSetNumCols(v, 1);
vdbe_metadata_set_col_name(v, 0, "rows deleted");
vdbe_metadata_set_col_type(v, 0, "integer");
- vdbe_metadata_set_col_nullability(v, 0, -1);
}
delete_from_cleanup:
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 159acec25..f1290e01c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -787,7 +787,6 @@ sqlInsert(Parse * pParse, /* Parser context */
column_name = "rows inserted";
vdbe_metadata_set_col_name(v, 0, column_name);
vdbe_metadata_set_col_type(v, 0, "integer");
- vdbe_metadata_set_col_nullability(v, 0, -1);
}
insert_cleanup:
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index a6a5f25ce..00ecde0a9 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -122,7 +122,6 @@ vdbe_set_pragma_result_columns(struct Vdbe *v, const struct PragmaName *pragma)
for (int i = 0, j = pragma->iPragCName; i < n; ++i) {
vdbe_metadata_set_col_name(v, i, pragCName[j++]);
vdbe_metadata_set_col_type(v, i, pragCName[j++]);
- vdbe_metadata_set_col_nullability(v, i, -1);
}
}
@@ -169,10 +168,8 @@ vdbe_emit_pragma_status(struct Parse *parse)
sqlVdbeSetNumCols(v, 2);
vdbe_metadata_set_col_name(v, 0, "pragma_name");
vdbe_metadata_set_col_type(v, 0, "text");
- vdbe_metadata_set_col_nullability(v, 0, -1);
vdbe_metadata_set_col_name(v, 1, "pragma_value");
vdbe_metadata_set_col_type(v, 1, "integer");
- vdbe_metadata_set_col_nullability(v, 1, -1);
parse->nMem = 2;
for (int i = 0; i < ArraySize(aPragmaName); ++i) {
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 5b5981c46..39e897ba3 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -150,7 +150,6 @@ sqlPrepare(sql * db, /* Database handle. */
azColName[name_index]);
vdbe_metadata_set_col_type(sParse.pVdbe, i,
azColName[name_index + 1]);
- vdbe_metadata_set_col_nullability(sParse.pVdbe, i, -1);
}
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 73ce95eba..63f517e62 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1813,7 +1813,6 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
coll_id->name_len);
}
}
- vdbe_metadata_set_col_nullability(v, i, -1);
if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 489328960..c08777a2d 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -500,7 +500,6 @@ sqlUpdate(Parse * pParse, /* The parser context */
sqlVdbeSetNumCols(v, 1);
vdbe_metadata_set_col_name(v, 0, "rows updated");
vdbe_metadata_set_col_type(v, 0, "integer");
- vdbe_metadata_set_col_nullability(v, 0, -1);
}
update_cleanup:
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 2de41a70a..ef542efc3 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1849,6 +1849,7 @@ vdbe_metadata_delete(struct Vdbe *v)
void
sqlVdbeSetNumCols(Vdbe * p, int nResColumn)
{
+ int old_n = p->nResColumn;
vdbe_metadata_delete(p);
p->nResColumn = (u16) nResColumn;
p->metadata = (struct sql_column_metadata *)
@@ -1859,6 +1860,9 @@ sqlVdbeSetNumCols(Vdbe * p, int nResColumn)
"calloc", "metadata");
return;
}
+ for (int i = old_n; i < nResColumn; ++i)
+ p->metadata[i].nullable = -1;
+
}
==================================================================
> 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;
2. What is a point of having it :2 bits, if you don't
have other flags in the same byte? Due to alignment
this member anyway will become 1 byte.
More information about the Tarantool-patches
mailing list