From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 ED18446971A for ; Fri, 6 Dec 2019 03:00:15 +0300 (MSK) References: <3ab4973b2ba51caca2e6c211a66a5f6fdf6faff7.1574846892.git.korablev@tarantool.org> <29dc1d4c-215c-902a-0532-259814a15740@tarantool.org> <20191205115051.GA56807@tarantool.org> From: Vladislav Shpilevoy Message-ID: <53b2d81f-2287-8916-56e4-d985221e5ff1@tarantool.org> Date: Fri, 6 Dec 2019 01:00:14 +0100 MIME-Version: 1.0 In-Reply-To: <20191205115051.GA56807@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 Cc: tarantool-patches@dev.tarantool.org 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. 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.