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

[Vladislav Shpilevoy]

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.