From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 3EF704696C2 for ; Fri, 29 Nov 2019 01:42:00 +0300 (MSK) References: <3ab4973b2ba51caca2e6c211a66a5f6fdf6faff7.1574846892.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <29dc1d4c-215c-902a-0532-259814a15740@tarantool.org> Date: Thu, 28 Nov 2019 23:41:57 +0100 MIME-Version: 1.0 In-Reply-To: <3ab4973b2ba51caca2e6c211a66a5f6fdf6faff7.1574846892.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 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! You added a leading whitespace to the commit title. 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. 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. See 5 comments below. On 27/11/2019 13:15, Nikita Pettik wrote: > 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 > --- > src/box/execute.c | 18 +- > src/box/iproto_constants.h | 1 + > src/box/lua/execute.c | 5 + > src/box/lua/net_box.c | 7 +- > src/box/sql/select.c | 6 +- > src/box/sql/sqlInt.h | 3 + > src/box/sql/vdbe.h | 3 + > src/box/sql/vdbeInt.h | 5 + > src/box/sql/vdbeapi.c | 7 + > src/box/sql/vdbeaux.c | 7 + > test/box/sql-update-with-nested-select.result | 5 +- > test/sql/boolean.result | 425 ++++++++++++++--------- > test/sql/check-clear-ephemeral.result | 5 +- > test/sql/collation.result | 74 ++-- > test/sql/gh-3199-no-mem-leaks.result | 120 ++++--- > test/sql/gh2141-delete-trigger-drop-table.result | 20 +- > test/sql/gh2251-multiple-update.result | 10 +- > test/sql/iproto.result | 105 +++--- > test/sql/misc.result | 25 +- > test/sql/on-conflict.result | 20 +- > test/sql/persistency.result | 190 ++++++---- > test/sql/row-count.result | 25 +- > test/sql/sql-debug.result | 15 +- > test/sql/transition.result | 190 ++++++---- > test/sql/types.result | 105 +++--- > test/sql/update-with-nested-select.result | 5 +- > 26 files changed, 862 insertions(+), 539 deletions(-) > > 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. > { > 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. > + 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)); > @@ -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. > } > 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. > }; > > enum iproto_ballot_key { > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > 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 {}.