Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
@ 2021-08-30  6:13 Mergen Imeev via Tarantool-patches
  2021-08-31 19:46 ` Timur Safin via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-30  6:13 UTC (permalink / raw)
  To: tsafin; +Cc: tarantool-patches

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
---
https://github.com/tarantool/tarantool/issues/6376
https://github.com/tarantool/tarantool/tree/imeevma/gh-6376-fix-cmp-between-big-double-and-dec

 .../gh-6376-fix-incorrect-dec-inf-cmp.md      |  3 ++
 src/box/sql/mem.c                             |  4 +-
 test/sql-tap/engine.cfg                       |  1 +
 .../gh-6376-wrong-double-to-dec-cmp.test.lua  | 38 +++++++++++++++++++
 4 files changed, 44 insertions(+), 2 deletions(-)
 create mode 100644 changelogs/unreleased/gh-6376-fix-incorrect-dec-inf-cmp.md
 create mode 100755 test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua

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;
 			decimal_t dec;
 			decimal_t *d = decimal_from_double(&dec, b->u.r);
 			assert(d != NULL && d == &dec);
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 587adbed9..e4ec41edb 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -35,6 +35,7 @@
     "built-in-functions.test.lua": {
         "memtx": {"engine": "memtx"}
     },
+    "gh-6376-wrong-double-to-dec-cmp.test.lua": {},
     "gh-4077-iproto-execute-no-bind.test.lua": {},
     "gh-6375-assert-on-unsupported-ext.test.lua": {},
     "*": {
diff --git a/test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua b/test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua
new file mode 100755
index 000000000..edfc851a2
--- /dev/null
+++ b/test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua
@@ -0,0 +1,38 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(4)
+
+-- Make sure that the comparison between DECIMAL and large DOUBLE is correct.
+test:do_execsql_test(
+    "gh-6376-1",
+    [[
+        SELECT CAST(1 AS DECIMAL) < -1e40;
+    ]], {
+        false
+    })
+
+test:do_execsql_test(
+    "gh-6376-2",
+    [[
+        SELECT CAST(-1 AS DECIMAL) > -1e400;
+    ]], {
+        true
+    })
+
+test:do_execsql_test(
+    "gh-6376-3",
+    [[
+        SELECT CAST(1 AS DECIMAL) <= 1e40;
+    ]], {
+        true
+    })
+
+test:do_execsql_test(
+    "gh-6376-4",
+    [[
+        SELECT CAST(1 AS DECIMAL) >= 1e400;
+    ]], {
+        false
+    })
+
+test:finish_test()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  2021-08-30  6:13 [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE 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 11:40 ` Igor Munkin via Tarantool-patches
  2021-09-09 10:24 ` Kirill Yukhin via Tarantool-patches
  2 siblings, 1 reply; 8+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-08-31 19:46 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

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

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

Thanks,
Timur


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  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
  0 siblings, 1 reply; 8+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-01  8:52 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  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
  0 siblings, 1 reply; 8+ messages in thread
From: Safin Timur via Tarantool-patches @ 2021-09-07  9:28 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches



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

Тимур

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  2021-09-07  9:28     ` Safin Timur via Tarantool-patches
@ 2021-09-07 11:26       ` Igor Munkin via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-09-07 11:26 UTC (permalink / raw)
  To: Safin Timur, Mergen Imeev; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  2021-08-30  6:13 [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE Mergen Imeev via Tarantool-patches
  2021-08-31 19:46 ` Timur Safin 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
  2 siblings, 1 reply; 8+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-09-07 11:40 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Mergen,

Thanks for the patch! LGTM, with a few nits regarding the commit
message.

On 30.08.21, Mergen Imeev via Tarantool-patches wrote:
> 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.

Minor: Considering Vlad's review[1], I would explicitly mention the
order of the operands to be compared (DECIMAL is the left one, DOUBLE is
the right one). The opposite case works fine even prior to the patch.

Minor: It would be clearer, if you mention here that there are only 38
decimal digits in DECIMAL representation (hence, 1e38).

> 
> Closes #6376
> ---
> https://github.com/tarantool/tarantool/issues/6376
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6376-fix-cmp-between-big-double-and-dec
> 
>  .../gh-6376-fix-incorrect-dec-inf-cmp.md      |  3 ++
>  src/box/sql/mem.c                             |  4 +-
>  test/sql-tap/engine.cfg                       |  1 +
>  .../gh-6376-wrong-double-to-dec-cmp.test.lua  | 38 +++++++++++++++++++
>  4 files changed, 44 insertions(+), 2 deletions(-)
>  create mode 100644 changelogs/unreleased/gh-6376-fix-incorrect-dec-inf-cmp.md
>  create mode 100755 test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua
> 

<snipped>

> -- 
> 2.25.1
> 

[1]: https://lists.tarantool.org/tarantool-patches/003f11f0-5ecc-2e69-dc89-47a34b5f24ac@tarantool.org/

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  2021-09-07 11:40 ` Igor Munkin via Tarantool-patches
@ 2021-09-09  7:39   ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 8+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-09  7:39 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Hi! Thank you for the review! My answers and new patch below.

On Tue, Sep 07, 2021 at 02:40:25PM +0300, Igor Munkin wrote:
> Mergen,
> 
> Thanks for the patch! LGTM, with a few nits regarding the commit
> message.
> 
> On 30.08.21, Mergen Imeev via Tarantool-patches wrote:
> > 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.
> 
> Minor: Considering Vlad's review[1], I would explicitly mention the
> order of the operands to be compared (DECIMAL is the left one, DOUBLE is
> the right one). The opposite case works fine even prior to the patch.
> 
Thanks, fixed.

> Minor: It would be clearer, if you mention here that there are only 38
> decimal digits in DECIMAL representation (hence, 1e38).
> 
Fixed.

> > 
> > Closes #6376
> > ---
> > https://github.com/tarantool/tarantool/issues/6376
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-6376-fix-cmp-between-big-double-and-dec
> > 
> >  .../gh-6376-fix-incorrect-dec-inf-cmp.md      |  3 ++
> >  src/box/sql/mem.c                             |  4 +-
> >  test/sql-tap/engine.cfg                       |  1 +
> >  .../gh-6376-wrong-double-to-dec-cmp.test.lua  | 38 +++++++++++++++++++
> >  4 files changed, 44 insertions(+), 2 deletions(-)
> >  create mode 100644 changelogs/unreleased/gh-6376-fix-incorrect-dec-inf-cmp.md
> >  create mode 100755 test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua
> > 
> 
> <snipped>
> 
> > -- 
> > 2.25.1
> > 
> 
> [1]: https://lists.tarantool.org/tarantool-patches/003f11f0-5ecc-2e69-dc89-47a34b5f24ac@tarantool.org/
> 
> -- 
> Best regards,
> IM


commit 04ce137be225ea4ee4ef83dd973a099a8cf385f6
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Mon Aug 23 09:34:23 2021 +0300

    sql: fix comparison between DECIMAL and big DOUBLE
    
    This patch fixes the comparison between DECIMAL as a left value and
    DOUBLE greater than or equal to 1e38 or less than or equal to -1e38 as a
    right value. Any DOUBLE value greater than or equal to 1e38 is now
    greater than any DECIMAL value, and a DOUBLE value less than or equal to
    -1e38 is less than any DECIMAL value. This is because our decimal cannot
    contain more than 38 digits.
    
    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 48755a017..b7af00723 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2452,9 +2452,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;
 			decimal_t dec;
 			decimal_t *d = decimal_from_double(&dec, b->u.r);
 			assert(d != NULL && d == &dec);
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index c35d1dced..a6f03307f 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -36,6 +36,7 @@
         "memtx": {"engine": "memtx"}
     },
     "gh-6157-unnecessary-free-on-string.test.lua": {},
+    "gh-6376-wrong-double-to-dec-cmp.test.lua": {},
     "gh-4077-iproto-execute-no-bind.test.lua": {},
     "gh-6375-assert-on-unsupported-ext.test.lua": {},
     "*": {
diff --git a/test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua b/test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua
new file mode 100755
index 000000000..edfc851a2
--- /dev/null
+++ b/test/sql-tap/gh-6376-wrong-double-to-dec-cmp.test.lua
@@ -0,0 +1,38 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(4)
+
+-- Make sure that the comparison between DECIMAL and large DOUBLE is correct.
+test:do_execsql_test(
+    "gh-6376-1",
+    [[
+        SELECT CAST(1 AS DECIMAL) < -1e40;
+    ]], {
+        false
+    })
+
+test:do_execsql_test(
+    "gh-6376-2",
+    [[
+        SELECT CAST(-1 AS DECIMAL) > -1e400;
+    ]], {
+        true
+    })
+
+test:do_execsql_test(
+    "gh-6376-3",
+    [[
+        SELECT CAST(1 AS DECIMAL) <= 1e40;
+    ]], {
+        true
+    })
+
+test:do_execsql_test(
+    "gh-6376-4",
+    [[
+        SELECT CAST(1 AS DECIMAL) >= 1e400;
+    ]], {
+        false
+    })
+
+test:finish_test()

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE
  2021-08-30  6:13 [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE Mergen Imeev via Tarantool-patches
  2021-08-31 19:46 ` Timur Safin via Tarantool-patches
  2021-09-07 11:40 ` Igor Munkin via Tarantool-patches
@ 2021-09-09 10:24 ` Kirill Yukhin via Tarantool-patches
  2 siblings, 0 replies; 8+ messages in thread
From: Kirill Yukhin via Tarantool-patches @ 2021-09-09 10:24 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hello,

On 30 авг 09:13, Mergen Imeev via Tarantool-patches wrote:
> 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
> ---
> https://github.com/tarantool/tarantool/issues/6376
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6376-fix-cmp-between-big-double-and-dec

I've checked your patch into master.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-09 10:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30  6:13 [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE 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
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox