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

[Mergen Imeev]

Thank you for the review! My answer below.

On Mon, Oct 04, 2021 at 11:53:50PM +0200, Vladislav Shpilevoy wrote:
> Nicely done!
> 
> On 01.10.2021 14:48, imeevma at tarantool.org wrote:
> > Part of #4145
> > ---
> >  src/box/sql/func.c | 64 +++++++++++++++++++---------------------------
> >  1 file changed, 26 insertions(+), 38 deletions(-)
> > 
> > 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.