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

[Nikita Pettik <korablev@tarantool.org>]

On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 3 comments below.
> 
> On 11/12/2019 14:44, Nikita Pettik wrote:
> > If result set contains column which features attached sequence
> > (AUTOINCREMENT in terms of SQL) then meta-information will contain
> > corresponding field ('is_autoicrement' : boolean) in response.
> > 
> > Part of #4407
> > ---
> > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> > index 644b373c9..88ef4ff78 100644
> > --- a/src/box/lua/net_box.c
> > +++ b/src/box/lua/net_box.c
> > @@ -651,11 +651,15 @@ decode_metadata_optional(struct lua_State *L, const char **data,
> >  			const char *coll = mp_decode_str(data, &len);
> >  			lua_pushlstring(L, coll, len);
> >  			lua_setfield(L, -2, "collation");
> > -		} else {
> > -			assert(key == IPROTO_FIELD_IS_NULLABLE);
> > +		} else if (key == IPROTO_FIELD_IS_NULLABLE) {
> >  			bool is_nullable = mp_decode_bool(data);
> >  			lua_pushboolean(L, is_nullable);
> >  			lua_setfield(L, -2, "is_nullable");
> > +		} else {
> > +			assert(key == IPROTO_FIELD_IS_AUTOINCREMENT);
> > +			bool autoincrement = mp_decode_bool(data);
> 
> 1. autoincrement -> is_autoincrement.

Fixed.
 
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index 73ce95eba..d92da4d8e 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1845,6 +1845,12 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> >  					space_def->fields[iCol].is_nullable;
> >  				vdbe_metadata_set_col_nullability(v, i,
> >  								  is_nullable);
> > +				if (pTabList->a[j].space->sequence != NULL) {
> > +					int afno =
> > +						pTabList->a[j].space->sequence_fieldno;
> > +					if (afno == iCol)
> > +						vdbe_metadata_set_col_autoincrement(v, i);
> > +				}
> >  			}
> >  		} else {
> >  			const char *z = NULL;
> 
> 2. Consider this refactoring to make it more accurate:
> 
> ============================================================================
> 
> @@ -1822,7 +1822,8 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  					break;
>  			}
>  			assert(j < pTabList->nSrc);
> -			struct space_def *space_def = pTabList->a[j].space->def;
> +			struct space *space = pTabList->a[j].space;
> +			struct space_def *space_def = space->def;
>  			assert(iCol >= 0 && iCol < (int)space_def->field_count);
>  			zCol = space_def->fields[iCol].name;
>  			const char *name = NULL;
> @@ -1845,12 +1846,9 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  					space_def->fields[iCol].is_nullable;
>  				vdbe_metadata_set_col_nullability(v, i,
>  								  is_nullable);
> -				if (pTabList->a[j].space->sequence != NULL) {
> -					int afno =
> -						pTabList->a[j].space->sequence_fieldno;
> -					if (afno == iCol)
> -						vdbe_metadata_set_col_autoincrement(v, i);
> -				}
> +				if (space->sequence != NULL &&
> +				    space->sequence_fieldno == iCol)
> +					vdbe_metadata_set_col_autoincrement(v, i);
>  			}
>  		} else {
>  			const char *z = NULL;
> 

Applied.

> ============================================================================
> 
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index 2de41a70a..e3672097c 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1911,6 +1911,13 @@ vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable)
> >  	p->metadata[idx].nullable = nullable;
> >  }
> >  
> > +void
> > +vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx)
> > +{
> > +	assert(idx < p->nResColumn);
> > +	p->metadata[idx].is_actoincrement = 1;
> 
> 3. Why 1 instead of true?

Idk, replaced with boolean.