Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Safin Timur <tsafin@tarantool.org>, 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 14:26:56 +0300
Message-ID: <20210907112656.GW5743@tarantool.org> (raw)
In-Reply-To: <7b0d7a1f-b307-dacd-1f35-200c043e6181@tarantool.org>

Hi there,

On 07.09.21, Safin Timur via Tarantool-patches wrote:
> 
> 
> 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 
> или что-то вроде того.
> - (ну или сделать и второе и третье - по желанию).

Как это улучшение относится к 2-строчному патчу по конкретному багосу?

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

Я не был ни в mem.c, ни в decimal.c, но все равно понял в чем патч (есть
же описание в commit message). Единственное, о чем бы стоило упомянуть
дополнительно -- порядок операндов (обратный случай работал и до этого,
как писал Влад в своем ревью).

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

Причем тут спешка и рефакторинг? Патч просто переставляет return-ы
местами (то, что затронуты строки с константами -- побочный эффект
diff-а). Зачем еще что-то менять в рамках этого патча, который решает
свою проблему?

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

Какую проблему это решит, без общего рефакторинга? Имхо, это не тот
билет, в рамках которого стоит разводить такую активность.

> - создаёшь тикет на короткий рефакторинг таких decimal-related граничных 
> проверок - перелопачиваешь весь mem.c

Что Мерген и предложил, AFAIU.

> 
> > 
> > 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. Мне не кажется, что 
> этот рефакторинг надо делать в одном шаге с граничными условиями, но 
> можно поместить в ту же серию.

В тему разговоров "для того парня" -- я лично не понял, что и где обсуждали
на предыдущих шагах. Можешь, пожалуйста, подсказать где стоит искать
обсуждение по теме переноса какой-то функциональности в decimal.c?

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

Опять же, текущие правки переставляют 2 return-а. Ты предлагаешь в этот
патч или даже в эту серию засунуть какой-то рефакторинг, связанный c
decimal в сиквеле. Почему объем рефакторинга (8 строчек кода) такой?
Имхо, если уж и разводить рефакторинг, то не на уровне константы vs
magiс numbers, а инкапсулировать эти проверки в decimal-specific модуле.

Мерген, нарисуй билет на эту активность, пожалуйста.

> 
> Тимур

-- 
Best regards,
IM

  reply	other threads:[~2021-09-07 11: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
2021-09-07  9:28     ` Safin Timur via Tarantool-patches
2021-09-07 11:26       ` Igor Munkin via Tarantool-patches [this message]
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=20210907112656.GW5743@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=imun@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