[tarantool-patches] Re: [PATCH 2/9] sql: disallow text values participate in sum() aggregate

[n.pettik]

>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index b86a95d9a..9adfeec67 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -1495,24 +1495,34 @@ static void
>> sumStep(sql_context * context, int argc, sql_value ** argv)
>> {
>> 	SumCtx *p;
>> -	int type;
>> 	assert(argc == 1);
>> 	UNUSED_PARAMETER(argc);
>> 	p = sql_aggregate_context(context, sizeof(*p));
>> -	type = sql_value_numeric_type(argv[0]);
>> -	if (p && type != SQL_NULL) {
>> -		p->cnt++;
>> -		if (type == SQL_INTEGER) {
>> -			i64 v = sql_value_int64(argv[0]);
>> -			p->rSum += v;
>> -			if ((p->approx | p->overflow) == 0
>> -			    && sqlAddInt64(&p->iSum, v)) {
>> -				p->overflow = 1;
>> -			}
>> -		} else {
>> -			p->rSum += sql_value_double(argv[0]);
>> -			p->approx = 1;
>> +	assert(p != NULL);
> 
> Why are you sure, that p != NULL? sql_aggregate_context()
> on first invocation allocates memory, and it can fail.

Well, theoretically speaking - yes, it can. You’ve fixed that
in review fix, so skipped.

>> +	int type = sql_value_type(argv[0]);
>> +	if (type == SQL_NULL)
>> +		return;
> 
> I've fixed the comment above and also I see, that sumStep is
> rewritten almost completely, so it is time to convert it to
> Tarantool style. See the diff below and on the branch in a
> separate commit.

Thx. Diff is OK, applied.