[Tarantool-patches] [PATCH v2 07/15] sql: rework SUM()
Mergen Imeev
imeevma at tarantool.org
Sat Sep 25 14:17:24 MSK 2021
Thank you for the review! My answer, diff and new patch below.
On Thu, Sep 23, 2021 at 12:48:13AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index b4461c6ee..b06955302 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -53,6 +53,29 @@
> > static struct mh_strnptr_t *built_in_functions = NULL;
> > static struct func_sql_builtin **functions;
> >
> > +/** Implementation of the SUM() function. */
> > +static void
> > +step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
> > +{
> > + assert(argc == 1);
> > + (void)argc;
> > + assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
> > + if (argv[0]->type == MEM_TYPE_NULL)
>
> Probably we should use mem_is_null() where possible. To avoid
> Mem members manual touch out of Mem.h and Mem.c. Here and in other
> places, other commits. Sorry I didn't notice it before.
Fixed here and in all similar patches. I will try to follow this rule in all
following patches.
Diff:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b06955302..4cc6f378b 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -59,10 +59,10 @@ step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
{
assert(argc == 1);
(void)argc;
- assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
- if (argv[0]->type == MEM_TYPE_NULL)
+ assert(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+ if (mem_is_null(argv[0]))
return;
- if (ctx->pMem->type == MEM_TYPE_NULL)
+ if (mem_is_null(ctx->pMem))
return mem_copy_as_ephemeral(ctx->pMem, argv[0]);
if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
ctx->is_aborted = true;
@@ -72,7 +72,7 @@ step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
static void
fin_sum(struct sql_context *ctx)
{
- assert(ctx->pMem->type == MEM_TYPE_NULL || mem_is_num(ctx->pMem));
+ assert(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
}
New patch:
commit 4e8b6f54349e1cfb53fab5dd39b41e77f9a7350a
Author: Mergen Imeev <imeevma at gmail.com>
Date: Thu Sep 9 17:50:21 2021 +0300
sql: rework SUM()
This patch makes SUM() accept DOUBLE values by default. Also, after this
patch SUM() will be able to work with DECIMAL values.
Part of #4145
Part of #6355
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b4461c6ee..4cc6f378b 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -53,6 +53,29 @@
static struct mh_strnptr_t *built_in_functions = NULL;
static struct func_sql_builtin **functions;
+/** Implementation of the SUM() function. */
+static void
+step_sum(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+ assert(argc == 1);
+ (void)argc;
+ assert(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+ if (mem_is_null(argv[0]))
+ return;
+ if (mem_is_null(ctx->pMem))
+ return mem_copy_as_ephemeral(ctx->pMem, argv[0]);
+ if (mem_add(ctx->pMem, argv[0], ctx->pMem) != 0)
+ ctx->is_aborted = true;
+}
+
+/** Finalizer for the SUM() function. */
+static void
+fin_sum(struct sql_context *ctx)
+{
+ assert(mem_is_null(ctx->pMem) || mem_is_num(ctx->pMem));
+ mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
+}
+
static const unsigned char *
mem_as_ustr(struct Mem *mem)
{
@@ -1654,17 +1677,6 @@ sum_step(struct sql_context *context, int argc, sql_value **argv)
context->is_aborted = true;
}
-static void
-sumFinalize(sql_context * context)
-{
- SumCtx *p;
- p = sql_aggregate_context(context, 0);
- if (p == NULL || p->count == 0)
- mem_set_null(context->pOut);
- else
- mem_copy_as_ephemeral(context->pOut, &p->mem);
-}
-
static void
avgFinalize(sql_context * context)
{
@@ -2113,8 +2125,9 @@ static struct sql_func_definition definitions[] = {
{"SUBSTR", 3,
{FIELD_TYPE_VARBINARY, FIELD_TYPE_INTEGER, FIELD_TYPE_INTEGER},
FIELD_TYPE_VARBINARY, substrFunc, NULL},
- {"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, sum_step, sumFinalize},
- {"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step, sumFinalize},
+ {"SUM", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_sum, fin_sum},
+ {"SUM", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_sum, fin_sum},
+ {"SUM", 1, {FIELD_TYPE_DECIMAL}, FIELD_TYPE_DECIMAL, step_sum, fin_sum},
{"TOTAL", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_DOUBLE, sum_step,
totalFinalize},
{"TOTAL", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, sum_step,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b101fc6c2..66ac2c4f3 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4207,7 +4207,6 @@ case OP_AggFinal: {
assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
struct Mem *pIn1 = &aMem[pOp->p1];
- assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
struct sql_context ctx;
memset(&ctx, 0, sizeof(ctx));
diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua
index 6fae811dc..8daf41c1d 100755
--- a/test/sql-tap/built-in-functions.test.lua
+++ b/test/sql-tap/built-in-functions.test.lua
@@ -1,6 +1,8 @@
#!/usr/bin/env tarantool
local test = require("sqltester")
-test:plan(52)
+test:plan(55)
+
+local dec = require('decimal')
--
-- Make sure that number of arguments check is checked properly for SQL built-in
@@ -455,7 +457,7 @@ test:do_test(
local res = {pcall(box.execute, [[SELECT SUM(?);]], {'1'})}
return {tostring(res[3])}
end, {
- "Type mismatch: can not convert string('1') to integer"
+ "Type mismatch: can not convert string('1') to double"
})
test:do_catchsql_test(
@@ -545,4 +547,33 @@ test:do_test(
{name = "COLUMN_2", type = "scalar"},
})
+-- Make sure SUM() accepts and returns DOUBLE by default.
+test:do_test(
+ "builtins-4.1.1",
+ function()
+ return box.execute([[SELECT SUM(?);]], {1}).metadata
+ end, {
+ {name = "COLUMN_1", type = "double"},
+ })
+
+test:do_test(
+ "builtins-4.1.2",
+ function()
+ local res = {pcall(box.execute, [[SELECT SUM(?);]], {-1ULL})}
+ return {tostring(res[3])}
+ end, {
+ "Type mismatch: can not convert integer(18446744073709551615) to double"
+ })
+
+-- Make sure SUM() works with DECIMAL properly.
+test:do_execsql_test(
+ "builtins-4.1.3",
+ [[
+ SELECT SUM(cast(column_2 as DECIMAL)) from (values(1), (123.432));
+ ]],
+ {
+ dec.new(124.432)
+ }
+)
+
test:finish_test()
More information about the Tarantool-patches
mailing list