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: Mon, 1 Nov 2021 13:11:44 +0300 [thread overview]
Message-ID: <20211101101144.GA227579@tarantool.org> (raw)
In-Reply-To: <d691bb9a-c05b-bb06-ee99-12148c59ffe8@tarantool.org>
Hi! Thank you the review! My answer below.
On Fri, Oct 29, 2021 at 12:11:11AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
> >>> + 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.
>
> I mean this:
>
> ====================
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index dbeb38bee..2a848be31 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -239,11 +239,10 @@ 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))
> + if (mem_is_null(arg) || mem_is_uint(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);
> + mem_set_uint(ctx->pOut, (uint64_t)-arg->u.i);
> }
> ====================
>
> Up to you.
This would work for aggregate function in some cases, but not here.
If we apply such diff the result of ABS() for UNSIGNED will be NULL.
next prev parent reply other threads:[~2021-11-01 10:11 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
2021-10-28 22:11 ` Vladislav Shpilevoy via Tarantool-patches
2021-11-01 10:11 ` Mergen Imeev via Tarantool-patches [this message]
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=20211101101144.GA227579@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