Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Timur Safin <tsafin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
Date: Wed, 1 Sep 2021 11:52:35 +0300	[thread overview]
Message-ID: <20210901085235.GA112149@tarantool.org> (raw)
In-Reply-To: <017301d79ea0$eba3de60$c2eb9b20$@tarantool.org>

Thank you for the review! My answer below.

On Tue, Aug 31, 2021 at 10:46:40PM +0300, Timur Safin wrote:
> > From: imeevma@tarantool.org <imeevma@tarantool.org>
> > Subject: [PATCH v1 1/1] sql: fix comparison between DECIMAL and big
> > DOUBLE
> > 
> > This patch fixes comparison between DECIMAL value and DOUBLE values
> > greater or equal to 1e38 or less or equal to -1e38. Now any DOUBLE
> > value
> > greater or equal to 1e38 is more than any DECIMAL value and DOUBLE
> > value less or equal to -1e38 is less than any DECIMAL value.
> > 
> > Closes #6376
> ...
> > diff --git a/changelogs/unreleased/gh-6376-fix-incorrect-dec-inf-
> > cmp.md b/changelogs/unreleased/gh-6376-fix-incorrect-dec-inf-cmp.md
> > new file mode 100644
> > index 000000000..70de655f1
> > --- /dev/null
> > +++ b/changelogs/unreleased/gh-6376-fix-incorrect-dec-inf-cmp.md
> > @@ -0,0 +1,3 @@
> > +## bugfix/sql
> > +
> > +* Fixed wrong comparison between DECIMAL and large DOUBLE values
> > (gh-6376).
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 4c40f15dc..a3ab31af5 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -2451,9 +2451,9 @@ mem_cmp_num(const struct Mem *a, const struct
> > Mem *b)
> >  		}
> >  		case MEM_TYPE_DOUBLE: {
> >  			if (b->u.r >= 1e38)
> > -				return 1;
> > -			if (b->u.r <= -1e38)
> >  				return -1;
> > +			if (b->u.r <= -1e38)
> > +				return 1;
> 
> Well, while we are here. I do understand that these kind of constants
> already spreading all corners of mem.c when you deal with decimals,
> but it's not entirely clear that this all about DECIMAL_MAX_DIGITS
> limitation in our decimal implementation. 
> And beyond all of that - hardcoded constant are evil. Could you please
> introduce any symbolic constant defines for such DECIMAL_MAX_DIGITS-
> derivative values? And use them wherever possible.
> 
> (I'm not insisting on fixing whole mem.c, that might be done separately,
> but at least this tricky case worth it)
> 
I agree that this is not good, but I think that addition of double constant for
decimal in SQL is not good idea. I think it is better to move all these
operations to decimal.c/.h. Could you fill an issue? If you cannot, I will do
it myself a bit later.

I also agree that I was wrong when I didn't introduce new functions for decimal,
but at that time I feared that I will spend too much time on tests for these
functions, and decided to leave this for later.

In general, I have quite a few questions about functions in decimal.c/.h and
I plan to ask them in the mentioned issue.

> >  			decimal_t dec;
> >  			decimal_t *d = decimal_from_double(&dec, b->u.r);
> >  			assert(d != NULL && d == &dec);
> 
> Thanks,
> Timur
> 

  reply	other threads:[~2021-09-01  8:52 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-30  6:13 Mergen Imeev via Tarantool-patches
2021-08-31 19:46 ` Timur Safin via Tarantool-patches
2021-09-01  8:52   ` Mergen Imeev via Tarantool-patches [this message]
2021-09-07  9:28     ` Safin Timur via Tarantool-patches
2021-09-07 11:26       ` Igor Munkin via Tarantool-patches
2021-09-07 11:40 ` Igor Munkin via Tarantool-patches
2021-09-09  7:39   ` Mergen Imeev via Tarantool-patches
2021-09-09 10:24 ` Kirill Yukhin 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=20210901085235.GA112149@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE' \
    /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