Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce custom aggregate functions

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch!

See 3 comments below. After you done with them, send to a next
reviewer right away.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index b69bf7fd6..cda872194 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -2060,9 +2060,12 @@ sql_func_find(struct Expr *expr)
>  		return NULL;
>  	}
>  	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
> -	if (func->def->param_count != n) {
> +	int argc = func->def->aggregate == FUNC_AGGREGATE_GROUP ?
> +		   func->def->param_count - 1 : func->def->param_count;

1. Why -1 for aggregates? I see it is somehow related to the
last argument being used for state, but it looks intricate. A
comment here would be enough.

> +	assert(argc >= 0);
> +	if (argc != n) {
>  		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
> -			 tt_sprintf("%d", func->def->param_count), n);
> +			 tt_sprintf("%d", argc), n);
>  		return NULL;
>  	}
>  	return func;> diff --git a/test/sql-tap/gh-2579-custom-aggregate.test.lua b/test/sql-tap/gh-2579-custom-aggregate.test.lua
> new file mode 100755
> index 000000000..213e2e870
> --- /dev/null
> +++ b/test/sql-tap/gh-2579-custom-aggregate.test.lua
> @@ -0,0 +1,113 @@
> +#!/usr/bin/env tarantool
> +local build_path = os.getenv("BUILDDIR")
> +package.cpath = build_path..'/test/sql-tap/?.so;'..build_path..'/test/sql-tap/?.dylib;'..package.cpath

2. Lets split it at 80th line column.

> +
> +local test = require("sqltester")
> +test:plan(5)
> +
> +test:execsql([[
> +        CREATE TABLE t (i INT PRIMARY KEY);
> +        INSERT INTO t VALUES(1), (2), (3), (4), (5);
> +    ]])

3. You probably forgot to left shift the expression for one tab.