[Tarantool-patches] [PATCH v4 11/16] sql: refactor COUNT() function

[Mergen Imeev]

Thank you for the review! My answer below.

On Mon, Oct 11, 2021 at 11:51:07PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> >>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> >>> index 94ec811ef..384c68be8 100644
> >>> --- a/src/box/sql/func.c
> >>> +++ b/src/box/sql/func.c
> >>> @@ -154,6 +154,29 @@ fin_avg(struct sql_context *ctx)
> >>>  		ctx->is_aborted = true;
> >>>  }
> >>>  
> >>> +/** Implementation of the COUNT() function. */
> >>> +static void
> >>> +step_count(struct sql_context *ctx, int argc, struct Mem **argv)
> >>> +{
> >>> +	assert(argc == 0 || argc == 1);
> >>> +	if (mem_is_null(ctx->pMem))
> >>> +		mem_set_uint(ctx->pMem, 0);
> >>
> >> Would be nice to have a 'begin' step for the aggregation
> >> functions. This would allow to eliminate these 'if is null'
> >> ifs in some step functions in favor of having +1 virtual 'begin'
> >> call in the beginning.
> >>
> >> Do you think it would simplify/speed up things? If you agree,
> >> could you please create a ticket for that? As 'good first issue'
> >> even.
> >>
> >> If don't agree, then ignore this comment.
> > I think this is a good idea, but I don't see a suitable way to implement it. The
> > problem is that in case we receive NULL as an argument, we will have NULL in
> > pMem/pOut after the first step, and we still have to call begin() again or check
> > with 'if'. And there is no way to determine how much NULLs will there be.
> 
> There are functions where it does not matter. For example,
> TOTAL() never returns NULL. You could initialize its
> accumulator with 0 from the beginning and drop
> 
> 	mem_set_double(ctx->pMem, 0.0);
> 
> from step_total() and fin_total().
> 
> The same for COUNT() and mem_set_uint(ctx->pMem, 0). We have
> more functions like these AFAIR.
Ok, understood. I filled an issue:
https://github.com/tarantool/tarantool/issues/6533