[Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
Igor Munkin
imun at tarantool.org
Tue Sep 7 14:26:56 MSK 2021
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 at tarantool.org <imeevma at 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
More information about the Tarantool-patches
mailing list