Re: [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.