From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 CA64946971A for ; Fri, 6 Dec 2019 15:49:25 +0300 (MSK) Date: Fri, 6 Dec 2019 15:49:25 +0300 From: Nikita Pettik Message-ID: <20191206124925.GA36787@tarantool.org> References: <3ab4973b2ba51caca2e6c211a66a5f6fdf6faff7.1574846892.git.korablev@tarantool.org> <29dc1d4c-215c-902a-0532-259814a15740@tarantool.org> <20191205115051.GA56807@tarantool.org> <53b2d81f-2287-8916-56e4-d985221e5ff1@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <53b2d81f-2287-8916-56e4-d985221e5ff1@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 06 Dec 01:00, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > On 05/12/2019 12:50, Nikita Pettik wrote: > > 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. > > There is a still actual reason - incremental alter. When you > have a space format and an index both specifying is_nullable, > sometimes you need a way to change the is_nullable value. And > if it would need to be the same everywhere, you would need to > drop all the related indexes, change nullability in the space > format, and recreate the indexes. This is long because of > indexes rebuilding. > > With the current implementation you can change nullability without > drop of the indexes. Step by step. Thanks for the explanation. > And it has nothing to do with transactional ddl. > > >>> 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. > > > Well, then you need to omit it in sql_column_is_nullable() too, because > it returns 3 values. Fair, renamed.