[Tarantool-patches] [PATCH v2 11/15] sql: rework MIN() and MAX()
Mergen Imeev
imeevma at tarantool.org
Sat Sep 25 14:36:48 MSK 2021
Thank you for the review! I replaced manual checks of MEM type by mem_is_*()
functions. Diff and new patch below.
On Tue, Sep 21, 2021 at 01:59:26PM +0300, Mergen Imeev via Tarantool-patches wrote:
> This patch does some refactoring for the SQL built-in aggregate
> functions MIN() and MAX().
>
> Part of #4145
> ---
> src/box/sql/func.c | 162 +++++++++++++++++++--------------------------
> src/box/sql/vdbe.c | 1 -
> 2 files changed, 69 insertions(+), 94 deletions(-)
>
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 113df423a..f699aa927 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -173,6 +173,46 @@ fin_count(struct sql_context *ctx)
> mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
> }
>
> +/** Implementation of the MIN() and MAX() functions. */
> +static void
> +step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
> +{
> + assert(argc == 1);
> + (void)argc;
> + if (argv[0]->type == MEM_TYPE_NULL) {
> + if (ctx->pMem->type != MEM_TYPE_NULL)
> + ctx->skipFlag = 1;
> + return;
> + }
> + if (ctx->pMem->type == MEM_TYPE_NULL) {
> + if (mem_copy(ctx->pMem, argv[0]) != 0)
> + ctx->is_aborted = true;
> + return;
> + }
> +
> + uint32_t flags = ((struct func_sql_builtin *)ctx->func)->flags;
> + bool is_max = (flags & SQL_FUNC_MAX) != 0;
> + /*
> + * This step function is used for both the min() and max() aggregates,
> + * the only difference between the two being that the sense of the
> + * comparison is inverted.
> + */
> + int cmp = mem_cmp_scalar(ctx->pMem, argv[0], ctx->coll);
> + if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
> + if (mem_copy(ctx->pMem, argv[0]) != 0)
> + ctx->is_aborted = true;
> + return;
> + }
> + ctx->skipFlag = 1;
> +}
> +
> +/** Finalizer for the MIN() and MAX() functions. */
> +static void
> +fin_minmax(struct sql_context *ctx)
> +{
> + mem_copy(ctx->pOut, ctx->pMem);
> +}
> +
> static const unsigned char *
> mem_as_ustr(struct Mem *mem)
> {
> @@ -220,16 +260,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
> mem_set_uuid(ctx->pOut, &uuid);
> }
>
> -/*
> - * Indicate that the accumulator load should be skipped on this
> - * iteration of the aggregate loop.
> - */
> -static void
> -sqlSkipAccumulatorLoad(sql_context * context)
> -{
> - context->skipFlag = 1;
> -}
> -
> /*
> * Implementation of the non-aggregate min() and max() functions
> */
> @@ -1727,60 +1757,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
> }
> }
>
> -/*
> - * Routines to implement min() and max() aggregate functions.
> - */
> -static void
> -minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
> -{
> - Mem *pArg = (Mem *) argv[0];
> - Mem *pBest;
> - UNUSED_PARAMETER(NotUsed);
> -
> - struct func_sql_builtin *func =
> - (struct func_sql_builtin *)context->func;
> - pBest = sql_context_agg_mem(context);
> - if (!pBest)
> - return;
> -
> - if (mem_is_null(argv[0])) {
> - if (!mem_is_null(pBest))
> - sqlSkipAccumulatorLoad(context);
> - } else if (!mem_is_null(pBest)) {
> - struct coll *pColl = context->coll;
> - /*
> - * This step function is used for both the min()
> - * and max() aggregates, the only difference
> - * between the two being that the sense of the
> - * comparison is inverted.
> - */
> - bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
> - int cmp = mem_cmp_scalar(pBest, pArg, pColl);
> - if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
> - if (mem_copy(pBest, pArg) != 0)
> - context->is_aborted = true;
> - } else {
> - sqlSkipAccumulatorLoad(context);
> - }
> - } else {
> - pBest->db = sql_context_db_handle(context);
> - if (mem_copy(pBest, pArg) != 0)
> - context->is_aborted = true;
> - }
> -}
> -
> -static void
> -minMaxFinalize(sql_context * context)
> -{
> - struct Mem *mem = context->pMem;
> - struct Mem *res;
> - if (mem_get_agg(mem, (void **)&res) != 0)
> - return;
> - if (!mem_is_null(res))
> - sql_result_value(context, res);
> - mem_destroy(res);
> -}
> -
> /*
> * group_concat(EXPR, ?SEPARATOR?)
> */
> @@ -2052,35 +2028,35 @@ static struct sql_func_definition definitions[] = {
> {"LOWER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, LowerICUFunc,
> NULL},
>
> - {"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxStep,
> - minMaxFinalize},
> - {"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxStep,
> - minMaxFinalize},
> - {"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxStep,
> - minMaxFinalize},
> - {"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxStep,
> - minMaxFinalize},
> - {"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxStep,
> - minMaxFinalize},
> - {"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxStep,
> - minMaxFinalize},
> - {"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxStep,
> - minMaxFinalize},
> -
> - {"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxStep,
> - minMaxFinalize},
> - {"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxStep,
> - minMaxFinalize},
> - {"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxStep,
> - minMaxFinalize},
> - {"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxStep,
> - minMaxFinalize},
> - {"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxStep,
> - minMaxFinalize},
> - {"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxStep,
> - minMaxFinalize},
> - {"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxStep,
> - minMaxFinalize},
> + {"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
> + fin_minmax},
> + {"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
> + fin_minmax},
> + {"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
> + fin_minmax},
> + {"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
> + fin_minmax},
> + {"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
> + fin_minmax},
> + {"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
> + fin_minmax},
> + {"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
> + fin_minmax},
> +
> + {"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
> + fin_minmax},
> + {"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
> + fin_minmax},
> + {"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
> + fin_minmax},
> + {"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
> + fin_minmax},
> + {"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
> + fin_minmax},
> + {"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
> + fin_minmax},
> + {"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
> + fin_minmax},
>
> {"NULLIF", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
> nullifFunc, NULL},
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a74ee29ea..8804e3d18 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4169,7 +4169,6 @@ case OP_AggStep: {
> }
> #endif
>
> - pMem->n++;
> mem_create(&t);
> pCtx->pOut = &t;
> pCtx->is_aborted = false;
> --
> 2.25.1
>
Diff:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 59cb9aca8..3708440e3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -183,12 +183,12 @@ step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
{
assert(argc == 1);
(void)argc;
- if (argv[0]->type == MEM_TYPE_NULL) {
- if (ctx->pMem->type != MEM_TYPE_NULL)
+ if (mem_is_null(argv[0])) {
+ if (!mem_is_null(ctx->pMem))
ctx->skipFlag = 1;
return;
}
- if (ctx->pMem->type == MEM_TYPE_NULL) {
+ if (mem_is_null(ctx->pMem)) {
if (mem_copy(ctx->pMem, argv[0]) != 0)
ctx->is_aborted = true;
return;
New patch:
commit 898e3104cedbe5f91c04d70a4d8436dbb9dd094d
Author: Mergen Imeev <imeevma at gmail.com>
Date: Thu Sep 9 18:32:19 2021 +0300
sql: rework MIN() and MAX()
This patch does some refactoring for the SQL built-in aggregate
functions MIN() and MAX().
Part of #4145
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 00db6512a..3708440e3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -177,6 +177,46 @@ fin_count(struct sql_context *ctx)
mem_copy_as_ephemeral(ctx->pOut, ctx->pMem);
}
+/** Implementation of the MIN() and MAX() functions. */
+static void
+step_minmax(struct sql_context *ctx, int argc, struct Mem **argv)
+{
+ assert(argc == 1);
+ (void)argc;
+ if (mem_is_null(argv[0])) {
+ if (!mem_is_null(ctx->pMem))
+ ctx->skipFlag = 1;
+ return;
+ }
+ if (mem_is_null(ctx->pMem)) {
+ if (mem_copy(ctx->pMem, argv[0]) != 0)
+ ctx->is_aborted = true;
+ return;
+ }
+
+ uint32_t flags = ((struct func_sql_builtin *)ctx->func)->flags;
+ bool is_max = (flags & SQL_FUNC_MAX) != 0;
+ /*
+ * This step function is used for both the min() and max() aggregates,
+ * the only difference between the two being that the sense of the
+ * comparison is inverted.
+ */
+ int cmp = mem_cmp_scalar(ctx->pMem, argv[0], ctx->coll);
+ if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
+ if (mem_copy(ctx->pMem, argv[0]) != 0)
+ ctx->is_aborted = true;
+ return;
+ }
+ ctx->skipFlag = 1;
+}
+
+/** Finalizer for the MIN() and MAX() functions. */
+static void
+fin_minmax(struct sql_context *ctx)
+{
+ mem_copy(ctx->pOut, ctx->pMem);
+}
+
static const unsigned char *
mem_as_ustr(struct Mem *mem)
{
@@ -224,16 +264,6 @@ sql_func_uuid(struct sql_context *ctx, int argc, struct Mem **argv)
mem_set_uuid(ctx->pOut, &uuid);
}
-/*
- * Indicate that the accumulator load should be skipped on this
- * iteration of the aggregate loop.
- */
-static void
-sqlSkipAccumulatorLoad(sql_context * context)
-{
- context->skipFlag = 1;
-}
-
/*
* Implementation of the non-aggregate min() and max() functions
*/
@@ -1731,60 +1761,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
}
}
-/*
- * Routines to implement min() and max() aggregate functions.
- */
-static void
-minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
-{
- Mem *pArg = (Mem *) argv[0];
- Mem *pBest;
- UNUSED_PARAMETER(NotUsed);
-
- struct func_sql_builtin *func =
- (struct func_sql_builtin *)context->func;
- pBest = sql_context_agg_mem(context);
- if (!pBest)
- return;
-
- if (mem_is_null(argv[0])) {
- if (!mem_is_null(pBest))
- sqlSkipAccumulatorLoad(context);
- } else if (!mem_is_null(pBest)) {
- struct coll *pColl = context->coll;
- /*
- * This step function is used for both the min()
- * and max() aggregates, the only difference
- * between the two being that the sense of the
- * comparison is inverted.
- */
- bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
- int cmp = mem_cmp_scalar(pBest, pArg, pColl);
- if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
- if (mem_copy(pBest, pArg) != 0)
- context->is_aborted = true;
- } else {
- sqlSkipAccumulatorLoad(context);
- }
- } else {
- pBest->db = sql_context_db_handle(context);
- if (mem_copy(pBest, pArg) != 0)
- context->is_aborted = true;
- }
-}
-
-static void
-minMaxFinalize(sql_context * context)
-{
- struct Mem *mem = context->pMem;
- struct Mem *res;
- if (mem_get_agg(mem, (void **)&res) != 0)
- return;
- if (!mem_is_null(res))
- sql_result_value(context, res);
- mem_destroy(res);
-}
-
/*
* group_concat(EXPR, ?SEPARATOR?)
*/
@@ -2056,35 +2032,35 @@ static struct sql_func_definition definitions[] = {
{"LOWER", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, LowerICUFunc,
NULL},
- {"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxStep,
- minMaxFinalize},
- {"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxStep,
- minMaxFinalize},
- {"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxStep,
- minMaxFinalize},
- {"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxStep,
- minMaxFinalize},
- {"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxStep,
- minMaxFinalize},
- {"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxStep,
- minMaxFinalize},
- {"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxStep,
- minMaxFinalize},
-
- {"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, minmaxStep,
- minMaxFinalize},
- {"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, minmaxStep,
- minMaxFinalize},
- {"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, minmaxStep,
- minMaxFinalize},
- {"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, minmaxStep,
- minMaxFinalize},
- {"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, minmaxStep,
- minMaxFinalize},
- {"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxStep,
- minMaxFinalize},
- {"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxStep,
- minMaxFinalize},
+ {"MAX", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
+ fin_minmax},
+ {"MAX", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
+ fin_minmax},
+ {"MAX", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
+ fin_minmax},
+ {"MAX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
+ fin_minmax},
+ {"MAX", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
+ fin_minmax},
+ {"MAX", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
+ fin_minmax},
+ {"MAX", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
+ fin_minmax},
+
+ {"MIN", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_minmax,
+ fin_minmax},
+ {"MIN", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_minmax,
+ fin_minmax},
+ {"MIN", 1, {FIELD_TYPE_NUMBER}, FIELD_TYPE_NUMBER, step_minmax,
+ fin_minmax},
+ {"MIN", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, step_minmax,
+ fin_minmax},
+ {"MIN", 1, {FIELD_TYPE_UUID}, FIELD_TYPE_UUID, step_minmax,
+ fin_minmax},
+ {"MIN", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, step_minmax,
+ fin_minmax},
+ {"MIN", 1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, step_minmax,
+ fin_minmax},
{"NULLIF", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR,
nullifFunc, NULL},
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 66ac2c4f3..ee07af9c2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4169,7 +4169,6 @@ case OP_AggStep: {
}
#endif
- pMem->n++;
mem_create(&t);
pCtx->pOut = &t;
pCtx->is_aborted = false;
More information about the Tarantool-patches
mailing list