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

[Vladislav Shpilevoy <v.shpilevoy@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.