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

[Nikita Pettik <korablev@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.