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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

> +			lua_pushboolean(L, autoincrement);
> +			lua_setfield(L, -2, "is_autoincrement");
>  		}
>  	}
>  }
> 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;

============================================================================

> 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?