Tarantool development patches archive
 help / color / mirror / Atom feed
From: Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Mergen Imeev <imeevma@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: Tue, 7 Sep 2021 12:28:23 +0300
Message-ID: <7b0d7a1f-b307-dacd-1f35-200c043e6181@tarantool.org> (raw)
In-Reply-To: <20210901085235.GA112149@tarantool.org>



On 01.09.2021 11:52, Mergen Imeev wrote:
> 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.

Ну вот давай посмотрим на этот код чужими глазами. Ты ведь понимаешь, 
что пишешь его не для себя, а "для того парня" и тут из контекста не 
всякому понятно, почему 1e38 при операциях с даблом. Ситуацию можно было 
бы улучшить несколькими способами:
- если волшебная константа используется один раз - то просто написать 
комментарий
- если использования магических констант несколько - то просто вынести в 
именованную константуЮ из названия которой за километр будет понятно, 
что это про лимиты нашей decimal реализации. Например, MAX_DECIMAL_FLOAT 
или что-то вроде того.
- (ну или сделать и второе и третье - по желанию).

Тут же нет ничего такого не сделано в прошлом и продолжается в 
настоящем, и простое чтение этого кода требует приложить дополнительные 
когнитивные усилия для того, чтобы понять что за захардкоженная 
константа из окружающего контекста. Причем, для того, чтобы понять это 
место в mem.c надо случайно уже побывать в decimal.c.
> 
> 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.

Да, тогда была спешка и на такое закрывали глаза, но давай всё же 
завяжем с такими практиками? (Понятно, что спешка будет всегда, но таки 
когда-то надо начинать. Благо у нас таки есть карт-бланш на 
кратковременные рефакторинги)

В данной ситуации можно приступить за нексоклько шагов:
- для данного патча (и только для него) вводим внутри mem.c 
макрос/именованную константу и использовать только её
- создаёшь тикет на короткий рефакторинг таких decimal-related граничных 
проверок - перелопачиваешь весь mem.c

> 
> 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);

Эта проблема была очевидной еще на предыдущих шагах да, и я, и Серёжа 
Петренко просили перенести этот код в decimal.c. Мне не кажется, что 
этот рефакторинг надо делать в одном шаге с граничными условиями, но 
можно поместить в ту же серию.

>>
>> Thanks,
>> Timur
>>

Если ты считаешь, что что-то можно сделать потом и коммитишься под это, 
то важно чтобы такой фоллоуап тикет был создан тобой, а не кем-то 
другим. Но в рамках обсуждаемых текущих правок, мне кажется что 
отдельный тикет - излишен, так как правки тривиальные (надо поправить 
всего 8 строчек кода в mem.c - come on!) и могут идти фоллоуапом 
релевантному патчу, когда пришли в какое-то место кода и огляделись.

Тимур

  reply	other threads:[~2021-09-07  9:28 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
2021-09-07  9:28     ` Safin Timur via Tarantool-patches [this message]
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=7b0d7a1f-b307-dacd-1f35-200c043e6181@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git