Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix comparison between DECIMAL and big DOUBLE

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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