From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 7934C4696C3 for ; Wed, 18 Dec 2019 03:29:45 +0300 (MSK) References: <38fabdf9592d40d8f90920eb85ffbef72c564da9.1576071711.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 18 Dec 2019 01:29:43 +0100 MIME-Version: 1.0 In-Reply-To: <38fabdf9592d40d8f90920eb85ffbef72c564da9.1576071711.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.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.