Re: [Tarantool-patches] [PATCH v4 38/53] sql: introduce mem_*_aggregate()

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 078de0e62..0211069c6 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -613,6 +613,29 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame)
>  	mem->field_type = field_type_MAX;
>  }
>  
> +int
> +mem_prepare_aggregate(struct Mem *mem, struct func *func, int size)

1. Why is this called 'prepare' when the other setters are called 'set'?
Maybe use 'set' here as well?

> +{
> +	if (size <= 0) {
> +		mem_clear(mem);
> +		return 0;
> +	}
> +	if (sqlVdbeMemGrow(mem, size, 0) != 0)
> +		return -1;
> +	memset(mem->z, 0, size);
> +	mem->n = size;
> +	mem->flags = MEM_Agg;
> +	mem->u.func = func;
> +	mem->field_type = field_type_MAX;
> +	return 0;
> +}
> +
> +void *
> +mem_get_aggregate(struct Mem *mem)
> +{
> +	return (void *)mem->z;

2. Void cast should work implicitly here I think. But what is
more interesting is why do you even need the getter? The other
mem types don't have getters for the union fields, and it is
fine I suppose. Would be too much. This one does not even check
if the type is an aggregate.