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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

> 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.

> +	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.

(Note, 'sql_value' below is not prepended with 'struct' because it
is a typedef - compiler curses).

==================================================================
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9adfeec67..0f9e228dc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1492,15 +1492,13 @@ struct SumCtx {
  * it overflows an integer.
  */
 static void
-sumStep(sql_context * context, int argc, sql_value ** argv)
+sum_step(struct sql_context *context, int argc, sql_value **argv)
 {
-	SumCtx *p;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	p = sql_aggregate_context(context, sizeof(*p));
-	assert(p != NULL);
+	struct SumCtx *p = sql_aggregate_context(context, sizeof(*p));
 	int type = sql_value_type(argv[0]);
-	if (type == SQL_NULL)
+	if (type == SQL_NULL || p == NULL)
 		return;
 	if (type != SQL_FLOAT && type != SQL_INTEGER) {
 		if (mem_apply_numeric_type(argv[0]) != 0) {
@@ -1514,10 +1512,10 @@ sumStep(sql_context * context, int argc, sql_value ** argv)
 	}
 	p->cnt++;
 	if (type == SQL_INTEGER) {
-		i64 v = sql_value_int64(argv[0]);
+		int64_t v = sql_value_int64(argv[0]);
 		p->rSum += v;
 		if ((p->approx | p->overflow) == 0 &&
-		    sqlAddInt64(&p->iSum, v)) {
+		    sqlAddInt64(&p->iSum, v) != 0) {
 			p->overflow = 1;
 		}
 	} else {
@@ -1870,9 +1868,12 @@ sqlRegisterBuiltinFunctions(void)
 		FUNCTION(zeroblob, 1, 0, 0, zeroblobFunc, FIELD_TYPE_SCALAR),
 		FUNCTION_COLL(substr, 2, 0, 0, substrFunc),
 		FUNCTION_COLL(substr, 3, 0, 0, substrFunc),
-		AGGREGATE(sum, 1, 0, 0, sumStep, sumFinalize, FIELD_TYPE_NUMBER),
-		AGGREGATE(total, 1, 0, 0, sumStep, totalFinalize, FIELD_TYPE_NUMBER),
-		AGGREGATE(avg, 1, 0, 0, sumStep, avgFinalize, FIELD_TYPE_NUMBER),
+		AGGREGATE(sum, 1, 0, 0, sum_step, sumFinalize,
+			  FIELD_TYPE_NUMBER),
+		AGGREGATE(total, 1, 0, 0, sum_step, totalFinalize,
+			  FIELD_TYPE_NUMBER),
+		AGGREGATE(avg, 1, 0, 0, sum_step, avgFinalize,
+			  FIELD_TYPE_NUMBER),
 		AGGREGATE2(count, 0, 0, 0, countStep, countFinalize,
 			   SQL_FUNC_COUNT, FIELD_TYPE_INTEGER),
 		AGGREGATE(count, 1, 0, 0, countStep, countFinalize,