From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v1 1/8] sql: refactor ABS() funcion Date: Wed, 20 Oct 2021 19:52:16 +0300 [thread overview] Message-ID: <20211020165216.GA203963@tarantool.org> (raw) In-Reply-To: <4dd3b621-96a6-5c23-0c92-3a3cf0f398ee@tarantool.org> Hi! Thank you for the review! My answers, diff and new patch below. On Fri, Oct 08, 2021 at 11:55:33PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > On 01.10.2021 18:29, imeevma@tarantool.org wrote: > > Part of #4145 > > --- > > src/box/sql/func.c | 83 +++++++++++++++++----------------------------- > > 1 file changed, 30 insertions(+), 53 deletions(-) > > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index b23636d7e..54b03f359 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -237,6 +237,32 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem *argv) > > ctx->is_aborted = true; > > } > > > > +/** Implementations of the ABS() function. */ > > +static void > > +func_abs_int(struct sql_context *ctx, int argc, struct Mem *argv) > > +{ > > + assert(argc == 1); > > + (void)argc; > > + struct Mem *arg = &argv[0]; > > + if (arg->type == MEM_TYPE_NULL) > > + return; > > 1. Need mem_is_null(). The same for the other places if there > are more. > Fixed here in all other places in this patch-set. > > + assert(mem_is_int(arg)); > > + uint64_t u = mem_is_uint(arg) ? arg->u.u : (uint64_t)-arg->u.i; > > 2. You could make return when mem_is_uint(). It would remove '?' and > mem_set_uint() which would calls mem_clear() inside. > I am not sure that I understood correctly. In case of argument being uint we can use mem_copy_as_ephemeral() instead of mem_set_uint(), but I am not sure if it would be better. > > + mem_set_uint(ctx->pOut, u); Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 5f90fb597..dbeb38bee 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -239,7 +239,7 @@ func_abs_int(struct sql_context *ctx, int argc, struct Mem *argv) assert(argc == 1); (void)argc; struct Mem *arg = &argv[0]; - if (arg->type == MEM_TYPE_NULL) + if (mem_is_null(arg)) return; assert(mem_is_int(arg)); uint64_t u = mem_is_uint(arg) ? arg->u.u : (uint64_t)-arg->u.i; @@ -252,9 +252,9 @@ func_abs_double(struct sql_context *ctx, int argc, struct Mem *argv) assert(argc == 1); (void)argc; struct Mem *arg = &argv[0]; - if (arg->type == MEM_TYPE_NULL) + if (mem_is_null(arg)) return; - assert(arg->type == MEM_TYPE_DOUBLE); + assert(mem_is_double(arg)); mem_set_double(ctx->pOut, arg->u.r < 0 ? -arg->u.r : arg->u.r); } New patch: commit 2cf5a2417954cb06bcc17cabf0636b6ba130cf8d Author: Mergen Imeev <imeevma@gmail.com> Date: Tue Sep 21 14:39:57 2021 +0300 sql: refactor ABS() funcion Part of #4145 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 0cf17572b..dbeb38bee 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -232,6 +232,32 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem *argv) ctx->is_aborted = true; } +/** Implementations of the ABS() function. */ +static void +func_abs_int(struct sql_context *ctx, int argc, struct Mem *argv) +{ + assert(argc == 1); + (void)argc; + struct Mem *arg = &argv[0]; + if (mem_is_null(arg)) + return; + assert(mem_is_int(arg)); + uint64_t u = mem_is_uint(arg) ? arg->u.u : (uint64_t)-arg->u.i; + mem_set_uint(ctx->pOut, u); +} + +static void +func_abs_double(struct sql_context *ctx, int argc, struct Mem *argv) +{ + assert(argc == 1); + (void)argc; + struct Mem *arg = &argv[0]; + if (mem_is_null(arg)) + return; + assert(mem_is_double(arg)); + mem_set_double(ctx->pOut, arg->u.r < 0 ? -arg->u.r : arg->u.r); +} + static const unsigned char * mem_as_ustr(struct Mem *mem) { @@ -395,57 +421,6 @@ lengthFunc(struct sql_context *context, int argc, struct Mem *argv) } } -/* - * Implementation of the abs() function. - * - * IMP: R-23979-26855 The abs(X) function returns the absolute value of - * the numeric argument X. - */ -static void -absFunc(struct sql_context *context, int argc, struct Mem *argv) -{ - assert(argc == 1); - UNUSED_PARAMETER(argc); - switch (sql_value_type(&argv[0])) { - case MP_UINT: { - sql_result_uint(context, mem_get_uint_unsafe(&argv[0])); - break; - } - case MP_INT: { - int64_t value = mem_get_int_unsafe(&argv[0]); - assert(value < 0); - sql_result_uint(context, -value); - break; - } - case MP_NIL:{ - /* IMP: R-37434-19929 Abs(X) returns NULL if X is NULL. */ - sql_result_null(context); - break; - } - case MP_BOOL: - case MP_BIN: - case MP_EXT: - case MP_ARRAY: - case MP_MAP: { - diag_set(ClientError, ER_INCONSISTENT_TYPES, "number", - mem_str(&argv[0])); - context->is_aborted = true; - return; - } - default:{ - /* - * Abs(X) returns 0.0 if X is a string or blob - * that cannot be converted to a numeric value. - */ - double rVal = mem_get_double_unsafe(&argv[0]); - if (rVal < 0) - rVal = -rVal; - sql_result_double(context, rVal); - break; - } - } -} - /** * Implementation of the position() function. * @@ -1930,8 +1905,10 @@ struct sql_func_definition { * function should be defined in succession. */ static struct sql_func_definition definitions[] = { - {"ABS", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, absFunc, NULL}, - {"ABS", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, absFunc, NULL}, + {"ABS", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, func_abs_int, + NULL}, + {"ABS", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, func_abs_double, + NULL}, {"AVG", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_avg, fin_avg}, {"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_avg, fin_avg}, {"CHAR", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_STRING, charFunc, NULL},
next prev parent reply other threads:[~2021-10-20 16:52 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-01 16:29 [Tarantool-patches] [PATCH v1 0/8] Rework standard function Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 1/8] sql: refactor ABS() funcion Mergen Imeev via Tarantool-patches 2021-10-08 21:55 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-20 16:52 ` Mergen Imeev via Tarantool-patches [this message] 2021-10-28 22:11 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-01 10:11 ` Mergen Imeev via Tarantool-patches 2021-11-01 13:37 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 2/8] sql: refactor CHAR_LENGTH() function Mergen Imeev via Tarantool-patches 2021-10-08 21:56 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-20 16:58 ` Mergen Imeev via Tarantool-patches 2021-10-28 22:11 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-01 10:20 ` Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 3/8] sql: refactor UPPER() and LOWER() functions Mergen Imeev via Tarantool-patches 2021-10-20 17:02 ` Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 4/8] sql: refactor NULLIF() function Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 5/8] sql: rework TRIM() function Mergen Imeev via Tarantool-patches 2021-10-20 17:05 ` Mergen Imeev via Tarantool-patches 2021-10-28 22:12 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-01 10:35 ` Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 6/8] sql: rework POSITION() function Mergen Imeev via Tarantool-patches 2021-10-08 21:58 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-20 17:08 ` Mergen Imeev via Tarantool-patches 2021-11-01 10:41 ` Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 7/8] sql: rework SUBSTR() function Mergen Imeev via Tarantool-patches 2021-10-08 22:02 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-20 17:15 ` Mergen Imeev via Tarantool-patches 2021-10-28 22:13 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-01 10:45 ` Mergen Imeev via Tarantool-patches 2021-10-01 16:29 ` [Tarantool-patches] [PATCH v1 8/8] sql: refactor LIKE() function Mergen Imeev via Tarantool-patches 2021-10-08 22:02 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-20 17:19 ` Mergen Imeev via Tarantool-patches 2021-11-01 10:48 ` Mergen Imeev via Tarantool-patches 2021-11-01 10:53 ` Mergen Imeev via Tarantool-patches 2021-10-04 13:32 ` [Tarantool-patches] [PATCH v1 0/8] Rework standard function Mergen Imeev via Tarantool-patches 2021-11-01 13:38 ` Vladislav Shpilevoy via Tarantool-patches 2021-11-11 10:45 Mergen Imeev via Tarantool-patches 2021-11-11 10:45 ` [Tarantool-patches] [PATCH v1 1/8] sql: refactor ABS() funcion Mergen Imeev via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211020165216.GA203963@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 1/8] sql: refactor ABS() funcion' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox