Re: [Tarantool-patches] [PATCH v2 12/15] sql: rework GROUP_CONCAT()

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

Thanks for the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index f699aa927..001a8641c 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -213,6 +213,52 @@ fin_minmax(struct sql_context *ctx)
>  	mem_copy(ctx->pOut, ctx->pMem);
>  }
>  
> +/** Implementation of the GROUP_CONCAT() function. */
> +static void
> +step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv)
> +{
> +	assert(argc == 1 || argc == 2);
> +	(void)argc;
> +	if (argv[0]->type == MEM_TYPE_NULL)
> +		return;
> +	assert(mem_is_str(argv[0]) || mem_is_bin(argv[0]));
> +	if (ctx->pMem->type == MEM_TYPE_NULL) {
> +		if (mem_copy_str(ctx->pMem, argv[0]->z, argv[0]->n) != 0)

1. What if the argument is zeroblob with no actual memory allocated yet?

> +			ctx->is_aborted = true;
> +		return;
> +	}
> +	const char *sep = NULL;
> +	int sep_len = 0;
> +	if (argc == 1) {
> +		sep = ",";
> +		sep_len = 1;
> +	} else if (argv[1]->type == MEM_TYPE_NULL) {
> +		sep = "";
> +		sep_len = 0;
> +	} else {
> +		assert(mem_is_same_type(argv[0], argv[0]));
> +		sep = argv[1]->z;
> +		sep_len = argv[1]->n;
> +	}
> +	if (sep_len > 0) {
> +		if (mem_append(ctx->pMem, sep, sep_len) != 0) {

2. Will it work if sep_len == 0? If yes, then I would propose to
drop the len check here and call the append always.

> +			ctx->is_aborted = true;
> +			return;
> +		}