[Tarantool-patches] [PATCH v4 10/16] sql: refactor AVG() function
Mergen Imeev
imeevma at tarantool.org
Tue Oct 5 12:48:06 MSK 2021
Thank you for the review! My answer below.
On Mon, Oct 04, 2021 at 11:53:19PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 8a9de1254..94ec811ef 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -102,6 +102,58 @@ fin_total(struct sql_context *ctx)
>
> <...>
>
> > +
> > +/** Finalizer for the AVG() function. */
> > +static void
> > +fin_avg(struct sql_context *ctx)
> > +{
> > + assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
> > + if (mem_is_null(ctx->pMem))
> > + return mem_set_null(ctx->pOut);
> > + struct Mem *tmp = (struct Mem *)ctx->pMem->z;
> > + uint32_t *count_val = (uint32_t *)(tmp + 1);
> > + struct Mem sum;
> > + mem_create(&sum);
> > + mem_copy_as_ephemeral(&sum, tmp);
> > + mem_destroy(tmp);
> > + struct Mem count;
> > + mem_create(&count);
> > + mem_set_uint(&count, *count_val);
> > + if (mem_div(&sum, &count, ctx->pOut) != 0)
> > + ctx->is_aborted = true;
>
> Consider this:
>
> ====================
> @@ -141,17 +141,14 @@ fin_avg(struct sql_context *ctx)
> assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
> if (mem_is_null(ctx->pMem))
> return mem_set_null(ctx->pOut);
> - struct Mem *tmp = (struct Mem *)ctx->pMem->z;
> - uint32_t *count_val = (uint32_t *)(tmp + 1);
> - struct Mem sum;
> - mem_create(&sum);
> - mem_copy_as_ephemeral(&sum, tmp);
> - mem_destroy(tmp);
> + struct Mem *sum = (struct Mem *)ctx->pMem->z;
> + uint32_t *count_val = (uint32_t *)(sum + 1);
> struct Mem count;
> mem_create(&count);
> mem_set_uint(&count, *count_val);
> if (mem_div(&sum, &count, ctx->pOut) != 0)
> ctx->is_aborted = true;
> + mem_destroy(sum);
> }
This will work, however, I think it will create some unnecessary restrictions
due to changes with pMem and pOut in a few patches. I suggest to apply part of
you diff with exception of mem_destroy(), which I sugget to replace by assert().
We have full control over this tmp/sum mem and we know, that there will be no
memory to free, so assert should be enough.
What do you think of this diff?
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index a811e55f9..8213704e9 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -141,16 +141,13 @@ fin_avg(struct sql_context *ctx)
assert(mem_is_null(ctx->pMem) || mem_is_bin(ctx->pMem));
if (mem_is_null(ctx->pMem))
return mem_set_null(ctx->pOut);
- struct Mem *tmp = (struct Mem *)ctx->pMem->z;
- uint32_t *count_val = (uint32_t *)(tmp + 1);
- struct Mem sum;
- mem_create(&sum);
- mem_copy_as_ephemeral(&sum, tmp);
- mem_destroy(tmp);
+ struct Mem *sum = (struct Mem *)ctx->pMem->z;
+ uint32_t *count_val = (uint32_t *)(sum + 1);
+ assert(!VdbeMemDynamic(sum) && sum->szMalloc == 0);
struct Mem count;
mem_create(&count);
mem_set_uint(&count, *count_val);
- if (mem_div(&sum, &count, ctx->pOut) != 0)
+ if (mem_div(sum, &count, ctx->pOut) != 0)
ctx->is_aborted = true;
}
More information about the Tarantool-patches
mailing list