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

[Nikita Pettik <korablev@tarantool.org>]

On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index 169814a2e..cc5891bb8 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -420,6 +420,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> >  		sqlVdbeSetNumCols(v, 1);
> >  		vdbe_metadata_set_col_name(v, 0, "rows deleted");
> >  		vdbe_metadata_set_col_type(v, 0, "integer");
> > +		vdbe_metadata_set_col_nullability(v, 0, -1);
> 
> 1. All these set_col_nullability(-1) look really doubtful. You
> don't set collation NULL explicitly, but you set nullability -1
> explicitly. Why?
> 
> I think it is much better to fill default values of newly allocated
> sql_column_metadata in sqlVdbeSetNumCols. See my diff (not on the
> branch):

Looks reasonable. Thx, applied.

> ==================================================================
> 
> > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> > index d3de5770b..92a50dd7b 100644
> > --- a/src/box/sql/vdbeInt.h
> > +++ b/src/box/sql/vdbeInt.h
> > @@ -350,6 +350,11 @@ struct sql_column_metadata {
> >  	char *name;
> >  	char *type;
> >  	char *collation;
> > +	/**
> > +	 * -1 is for any member of result set except for pure
> > +	 * columns: all other expressions are nullable by default.
> > +	 */
> > +	int8_t nullable : 2;
> 
> 2. What is a point of having it :2 bits, if you don't
> have other flags in the same byte? Due to alignment
> this member anyway will become 1 byte.

I guess it's an artifact of one of previous iteration of review fixes
or of the original patch. Anyway, removed this bit field.