Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction
@ 2021-07-10 14:33 Mergen Imeev via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() Mergen Imeev via Tarantool-patches
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

After addition of UUID to SQL a few new problems appeared. This patch fixes such
problems.

https://github.com/tarantool/tarantool/issues/6164
https://github.com/tarantool/tarantool/tree/imeevma/gh-6164-uuid-follow-ups

Changes in v2:
 - Fixed problem with wrong comparison in MIN()/MAX() functions.
 - Fixed problem with wrong comparison in ORDER BY.

Mergen Imeev (4):
  sql: introduce uuid to quote()
  sql: allow to bind uuid values
  sql: introduce mem_cmp_scalar()
  sql: introduce mem_cmp_msgpack()

 src/box/bind.c                                |   3 +
 src/box/bind.h                                |   5 +
 src/box/lua/execute.c                         |   5 +
 src/box/sql.c                                 |   9 +-
 src/box/sql/func.c                            |  38 +-
 src/box/sql/mem.c                             | 416 ++++++------------
 src/box/sql/mem.h                             |  33 +-
 src/box/sql/sqlInt.h                          |   5 +
 src/box/sql/vdbe.c                            |   2 +-
 src/box/sql/vdbeapi.c                         |  10 +
 src/box/sql/where.c                           |   6 +-
 test/sql-tap/engine.cfg                       |   3 +
 test/sql-tap/gh-6164-uuid-follow-ups.test.lua |  93 ++++
 13 files changed, 307 insertions(+), 321 deletions(-)
 create mode 100755 test/sql-tap/gh-6164-uuid-follow-ups.test.lua

-- 
2.25.1


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

* [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote()
  2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
@ 2021-07-10 14:33 ` Mergen Imeev via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values Mergen Imeev via Tarantool-patches
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

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

On 08.07.2021 00:41, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
>> diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
>> new file mode 100755
>> index 000000000..d8fa700ea
>> --- /dev/null
>> +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
>
> 1. The test runs 2 times with 2 engines. Please, make it work only
> once. It does not depend on an engine anyway.
>
Fixed.

>> @@ -0,0 +1,16 @@
>> +#!/usr/bin/env tarantool
>> +local test = require("sqltester")
>> +test:plan(1)
>> +
>> +box.execute([[select quote(cast('11111111-1111-1111-1111-111111111111' as uuid));]])
>
> 2. Why do you need this select? Isn't it exactly the same as the SELECT below?
>
I believe I used it for debug and forgot to remove. Removed.

>> +
>> +-- Make sure that function quote() can work with uuid.
>> +test:do_execsql_test(
>> +    "gh-6164-1",
>> +    [[
>> +        SELECT quote(cast('11111111-1111-1111-1111-111111111111' as uuid));
>> +    ]], {
>> +        '11111111-1111-1111-1111-111111111111'
>> +    })
>> +
>> +test:finish_test()


Diff:

diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 693a477b7..94b0bb1f6 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -20,6 +20,9 @@
     "gh-3332-tuple-format-leak.test.lua": {
         "memtx": {"engine": "memtx"}
     },
+    "gh-6164-uuid-follow-ups.test.lua": {
+        "memtx": {"engine": "memtx"}
+    },
     "gh-4077-iproto-execute-no-bind.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index d8fa700ea..a8f662f77 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -2,8 +2,6 @@
 local test = require("sqltester")
 test:plan(1)
 
-box.execute([[select quote(cast('11111111-1111-1111-1111-111111111111' as uuid));]])
-
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
     "gh-6164-1",


New patch:

commit ec87733ca387c72957ac3202df1a6b0547e20fdc
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Mon Jul 5 11:09:48 2021 +0300

    sql: introduce uuid to quote()
    
    Prior to this patch, built-in SQL function quote() could not work with
    uuid. It now returns a string representation of the received uuid.
    
    Part of #6164

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f93ae867d..aa565277c 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1098,8 +1098,8 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 {
    assert(argc == 1);
    UNUSED_PARAMETER(argc);
-   switch (sql_value_type(argv[0])) {
-   case MP_DOUBLE:{
+   switch (argv[0]->type) {
+   case MEM_TYPE_DOUBLE:{
            double r1, r2;
            char zBuf[50];
            r1 = mem_get_double_unsafe(argv[0]);
@@ -1113,14 +1113,20 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
                        SQL_TRANSIENT);
            break;
        }
-   case MP_UINT:
-   case MP_INT:{
+   case MEM_TYPE_UUID: {
+       char buf[UUID_STR_LEN + 1];
+       tt_uuid_to_string(&argv[0]->u.uuid, &buf[0]);
+       sql_result_text(context, buf, UUID_STR_LEN, SQL_TRANSIENT);
+       break;
+   }
+   case MEM_TYPE_UINT:
+   case MEM_TYPE_INT: {
            sql_result_value(context, argv[0]);
            break;
        }
-   case MP_BIN:
-   case MP_ARRAY:
-   case MP_MAP: {
+   case MEM_TYPE_BIN:
+   case MEM_TYPE_ARRAY:
+   case MEM_TYPE_MAP: {
            char *zText = 0;
            char const *zBlob = mem_as_bin(argv[0]);
            int nBlob = mem_len_unsafe(argv[0]);
@@ -1146,7 +1152,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
            }
            break;
        }
-   case MP_STR:{
+   case MEM_TYPE_STR: {
            int i, j;
            u64 n;
            const unsigned char *zArg = mem_as_ustr(argv[0]);
@@ -1174,7 +1180,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
            }
            break;
        }
-   case MP_BOOL: {
+   case MEM_TYPE_BOOL: {
        sql_result_text(context,
                SQL_TOKEN_BOOLEAN(mem_get_bool_unsafe(argv[0])),
                -1, SQL_TRANSIENT);
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 693a477b7..94b0bb1f6 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -20,6 +20,9 @@
     "gh-3332-tuple-format-leak.test.lua": {
         "memtx": {"engine": "memtx"}
     },
+    "gh-6164-uuid-follow-ups.test.lua": {
+        "memtx": {"engine": "memtx"}
+    },
     "gh-4077-iproto-execute-no-bind.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
new file mode 100755
index 000000000..a8f662f77
--- /dev/null
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -0,0 +1,14 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(1)
+
+-- Make sure that function quote() can work with uuid.
+test:do_execsql_test(
+    "gh-6164-1",
+    [[
+        SELECT quote(cast('11111111-1111-1111-1111-111111111111' as uuid));
+    ]], {
+        '11111111-1111-1111-1111-111111111111'
+    })
+
+test:finish_test()

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

* [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values
  2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() Mergen Imeev via Tarantool-patches
@ 2021-07-10 14:33 ` Mergen Imeev via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answers, diff and new patch below. However, this
diff not exactly right, since the patch also changed due to dropping of previous
patch of patch-set.

On 08.07.2021 00:44, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 4 comments below.
>
> On 05.07.2021 17:06, Mergen Imeev via Tarantool-patches wrote:
>> After this patch, uuid values can be binded like any other supported by
>
> 1. binded -> bound.
>
Fixed.

>> diff --git a/src/box/bind.h b/src/box/bind.h
>> index 568c558f3..20f3e7942 100644
>> --- a/src/box/bind.h
>> +++ b/src/box/bind.h
>> @@ -40,6 +40,8 @@ extern "C" {
>>  #include <stdlib.h>
>>  
>>  #include "msgpuck.h"
>> +#include "uuid/tt_uuid.h"
>> +#include "lib/core/mp_extension_types.h"
>
> 2. lib/core is already in the paths. You can omit it.
>
Fixed.

>> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
>> index aaae12e41..a9700350d 100644
>> --- a/src/box/sql/vdbeapi.c
>> +++ b/src/box/sql/vdbeapi.c
>> @@ -840,6 +840,17 @@ sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
>>      return sql_bind_zeroblob(pStmt, i, n);
>>  }
>>  
>> +int
>> +sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
>> +{
>> +    struct Vdbe *p = (struct Vdbe *)stmt;
>> +    if (vdbeUnbind(p, i) != 0)
>> +            return -1;
>> +    int rc = sql_bind_type(p, i, "uuid");
>> +    mem_set_uuid(&p->aVar[i - 1], uuid);
>
> 3. Why do you set UUID even if bind_type() failed?
>
It happened because I used sql_bind_uint64() as prototype and did not noticed
that there were an error. Fixed.

>> diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
>> index 8872f9f23..426717972 100755
>> --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
>> +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
>> @@ -32,4 +32,18 @@ test:do_execsql_test(
>>          true
>>      })
>>  
>> +-- Make sure that uuid value can be binded.
>> +box.execute('CREATE TABLE t(i INT PRIMARY KEY, a UUID);')
>> +box.execute('INSERT INTO t VALUES(1, ?);', {uuid});
>
> 4. Do you need a table for that? Can you just make 'SELECT ?' with
> the uuid argument?
>
True, fixed.

>> +
>> +test:do_execsql_test(
>> +    "gh-6164-4",
>> +    [[
>> +        SELECT * FROM t;
>> +    ]], {
>> +        1, uuid
>> +    })
>> +
>> +box.execute([[DROP TABLE t;]])
>> +
>>  test:finish_test()
>>


Diff:

commit daf89852f18bc3dbb3255dfd785182da5ef81e93
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jul 10 14:50:53 2021 +0300

    Review fix

diff --git a/src/box/bind.h b/src/box/bind.h
index 20f3e7942..143f010ce 100644
--- a/src/box/bind.h
+++ b/src/box/bind.h
@@ -41,7 +41,7 @@ extern "C" {
 
 #include "msgpuck.h"
 #include "uuid/tt_uuid.h"
-#include "lib/core/mp_extension_types.h"
+#include "mp_extension_types.h"
 
 struct sql_stmt;
 
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index a9700350d..8031ee0dc 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -844,11 +844,10 @@ int
 sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
 {
        struct Vdbe *p = (struct Vdbe *)stmt;
-       if (vdbeUnbind(p, i) != 0)
+       if (vdbeUnbind(p, i) != 0 || sql_bind_type(p, i, "uuid") != 0)
                return -1;
-       int rc = sql_bind_type(p, i, "uuid");
        mem_set_uuid(&p->aVar[i - 1], uuid);
-       return rc;
+       return 0;
 }
 
 int
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 822311237..4fc5052d8 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,8 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(2)
-
-local uuid = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')
+test:plan(4)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -14,17 +12,27 @@ test:do_execsql_test(
     })
 
 -- Make sure that uuid value can be binded.
-box.execute('CREATE TABLE t(i INT PRIMARY KEY, a UUID);')
-box.execute('INSERT INTO t VALUES(1, ?);', {uuid});
+local uuid1 = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')
+local uuid2 = require('uuid').fromstr('11111111-2222-1111-1111-111111111111')
+local uuid3 = require('uuid').fromstr('11111111-1111-3333-1111-111111111111')
+test:do_test(
+    "gh-6164-2",
+    function()
+        return box.execute([[SELECT ?;]], {uuid1}).rows[1][1]
+    end,
+    uuid1)
+test:do_test(
+    "gh-6164-3",
+    function()
+        return box.execute([[SELECT $2;]], {123, uuid2}).rows[1][1]
+    end,
+    uuid2)
 
-test:do_execsql_test(
+test:do_test(
     "gh-6164-4",
-    [[
-        SELECT * FROM t;
-    ]], {
-        1, uuid
-    })
-
-box.execute([[DROP TABLE t;]])
+    function()
+        return box.execute([[SELECT :two;]], {{[":two"] = uuid3}}).rows[1][1]
+    end,
+    uuid3)
 
 test:finish_test()



New patch:

commit 8faa40c89f027dbb9aa8c8d7551b6b1aa214ed88
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Mon Jul 5 11:57:35 2021 +0300

    sql: allow to bind uuid values
    
    After this patch, uuid values can be bound like any other supported by
    SQL values.
    
    Part of #6164

diff --git a/src/box/bind.c b/src/box/bind.c
index d45a0f9a7..734f65186 100644
--- a/src/box/bind.c
+++ b/src/box/bind.c
@@ -191,6 +191,9 @@ sql_bind_column(struct sql_stmt *stmt, const struct sql_bind *p,
        case MP_BIN:
                return sql_bind_blob64(stmt, pos, (const void *) p->s, p->bytes,
                                       SQL_STATIC);
+       case MP_EXT:
+               assert(p->ext_type == MP_UUID);
+               return sql_bind_uuid(stmt, pos, &p->uuid);
        default:
                unreachable();
        }
diff --git a/src/box/bind.h b/src/box/bind.h
index 568c558f3..143f010ce 100644
--- a/src/box/bind.h
+++ b/src/box/bind.h
@@ -40,6 +40,8 @@ extern "C" {
 #include <stdlib.h>
 
 #include "msgpuck.h"
+#include "uuid/tt_uuid.h"
+#include "mp_extension_types.h"
 
 struct sql_stmt;
 
@@ -59,6 +61,8 @@ struct sql_bind {
        uint32_t bytes;
        /** MessagePack type of the value. */
        enum mp_type type;
+       /** Subtype of MP_EXT type. */
+       enum mp_extension_type ext_type;
        /** Bind value. */
        union {
                bool b;
@@ -67,6 +71,7 @@ struct sql_bind {
                uint64_t u64;
                /** For string or blob. */
                const char *s;
+               struct tt_uuid uuid;
        };
 };
 
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 1b59b2e4a..62ccaf3f1 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -371,6 +371,10 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
                bind->s = mp_decode_bin(&field.sval.data, &bind->bytes);
                break;
        case MP_EXT:
+               if (field.ext_type == MP_UUID) {
+                       bind->uuid = *field.uuidval;
+                       break;
+               }
                diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA",
                         sql_bind_name(bind));
                return -1;
@@ -386,6 +390,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
                unreachable();
        }
        bind->type = field.type;
+       bind->ext_type = field.ext_type;
        lua_pop(L, lua_gettop(L) - idx);
        return 0;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index ef8dcd693..115c52f96 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -326,6 +326,8 @@ struct sql_vfs {
 #define SQL_LIMIT_LIKE_PATTERN_LENGTH       8
 #define SQL_LIMIT_TRIGGER_DEPTH             9
 
+struct tt_uuid;
+
 enum sql_ret_code {
        /** sql_step() has another row ready. */
        SQL_ROW = 1,
@@ -634,6 +636,9 @@ int
 sql_bind_zeroblob64(sql_stmt *, int,
                        sql_uint64);
 
+int
+sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid);
+
 /**
  * Return the number of wildcards that should be bound to.
  */
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index aaae12e41..8031ee0dc 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -840,6 +840,16 @@ sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
        return sql_bind_zeroblob(pStmt, i, n);
 }
 
+int
+sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
+{
+       struct Vdbe *p = (struct Vdbe *)stmt;
+       if (vdbeUnbind(p, i) != 0 || sql_bind_type(p, i, "uuid") != 0)
+               return -1;
+       mem_set_uuid(&p->aVar[i - 1], uuid);
+       return 0;
+}
+
 int
 sql_bind_parameter_count(const struct sql_stmt *stmt)
 {
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index a8f662f77..4fc5052d8 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(1)
+test:plan(4)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -11,4 +11,28 @@ test:do_execsql_test(
         '11111111-1111-1111-1111-111111111111'
     })
 
+-- Make sure that uuid value can be binded.
+local uuid1 = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')
+local uuid2 = require('uuid').fromstr('11111111-2222-1111-1111-111111111111')
+local uuid3 = require('uuid').fromstr('11111111-1111-3333-1111-111111111111')
+test:do_test(
+    "gh-6164-2",
+    function()
+        return box.execute([[SELECT ?;]], {uuid1}).rows[1][1]
+    end,
+    uuid1)
+test:do_test(
+    "gh-6164-3",
+    function()
+        return box.execute([[SELECT $2;]], {123, uuid2}).rows[1][1]
+    end,
+    uuid2)
+
+test:do_test(
+    "gh-6164-4",
+    function()
+        return box.execute([[SELECT :two;]], {{[":two"] = uuid3}}).rows[1][1]
+    end,
+    uuid3)
+
 test:finish_test()

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

* [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
  2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() Mergen Imeev via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values Mergen Imeev via Tarantool-patches
@ 2021-07-10 14:33 ` Mergen Imeev via Tarantool-patches
  2021-07-11 15:03   ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
  2021-07-15 20:44 ` [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Vladislav Shpilevoy via Tarantool-patches
  4 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch introduces the mem_cmp_scalar() function that compares two
MEMs using SCALAR rules. MEMs must be scalars. Prior to this patch, there was a
function that used SCALAR rules to compare two MEMs, but its design became
overly complex as new types appeared.

Part of #6164
---
 src/box/sql/func.c                            |  14 +-
 src/box/sql/mem.c                             | 140 ++++++++----------
 src/box/sql/mem.h                             |  10 +-
 src/box/sql/vdbe.c                            |   2 +-
 src/box/sql/where.c                           |   6 +-
 test/sql-tap/gh-6164-uuid-follow-ups.test.lua |  45 +++++-
 6 files changed, 128 insertions(+), 89 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index aa565277c..dc99bd390 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 	for (i = 1; i < argc; i++) {
 		if (mem_is_null(argv[i]))
 			return;
-		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
-		    0) {
-			testcase(mask == 0);
+		int res;
+		mem_cmp_scalar(argv[iBest], argv[i], &res, pColl);
+		if ((res ^ mask) >= 0)
 			iBest = i;
-		}
 	}
 	sql_result_value(context, argv[iBest]);
 }
@@ -1057,9 +1056,10 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
 {
 	struct coll *pColl = sqlGetFuncCollSeq(context);
 	UNUSED_PARAMETER(NotUsed);
-	if (sqlMemCompare(argv[0], argv[1], pColl) != 0) {
+	int res;
+	mem_cmp_scalar(argv[0], argv[1], &res, pColl);
+	if (res != 0)
 		sql_result_value(context, argv[0]);
-	}
 }
 
 /**
@@ -1827,7 +1827,7 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 		 * comparison is inverted.
 		 */
 		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
-		cmp = sqlMemCompare(pBest, pArg, pColl);
+		mem_cmp_scalar(pBest, pArg, &cmp, pColl);
 		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
 			mem_copy(pBest, pArg);
 		} else {
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 2595e2fd4..576596c9f 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -59,6 +59,40 @@ enum {
 	BUF_SIZE = 32,
 };
 
+enum mem_class {
+	MEM_CLASS_NULL,
+	MEM_CLASS_BOOL,
+	MEM_CLASS_NUMBER,
+	MEM_CLASS_STR,
+	MEM_CLASS_BIN,
+	MEM_CLASS_UUID,
+	mem_class_max,
+};
+
+static inline enum mem_class
+mem_type_class(enum mem_type type)
+{
+	switch (type) {
+	case MEM_TYPE_NULL:
+		return MEM_CLASS_NULL;
+	case MEM_TYPE_UINT:
+	case MEM_TYPE_INT:
+	case MEM_TYPE_DOUBLE:
+		return MEM_CLASS_NUMBER;
+	case MEM_TYPE_STR:
+		return MEM_CLASS_STR;
+	case MEM_TYPE_BIN:
+		return MEM_CLASS_BIN;
+	case MEM_TYPE_BOOL:
+		return MEM_CLASS_BOOL;
+	case MEM_TYPE_UUID:
+		return MEM_CLASS_UUID;
+	default:
+		break;
+	}
+	return mem_class_max;
+}
+
 bool
 mem_is_field_compatible(const struct Mem *mem, enum field_type type)
 {
@@ -2009,6 +2043,36 @@ mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result)
 	return 0;
 }
 
+int
+mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
+	       const struct coll *coll)
+{
+	enum mem_class class_a = mem_type_class(a->type);
+	enum mem_class class_b = mem_type_class(b->type);
+	if (class_a != class_b) {
+		*result = class_a - class_b;
+		return 0;
+	}
+	switch (class_a) {
+	case MEM_CLASS_NULL:
+		*result = 0;
+		return 0;
+	case MEM_CLASS_BOOL:
+		return mem_cmp_bool(a, b, result);
+	case MEM_CLASS_NUMBER:
+		return mem_cmp_num(a, b, result);
+	case MEM_CLASS_STR:
+		return mem_cmp_str(a, b, result, coll);
+	case MEM_CLASS_BIN:
+		return mem_cmp_bin(a, b, result);
+	case MEM_CLASS_UUID:
+		return mem_cmp_uuid(a, b, result);
+	default:
+		unreachable();
+	}
+	return 0;
+}
+
 /*
  * Both *pMem1 and *pMem2 contain string values. Compare the two values
  * using the collation sequence pColl. As usual, return a negative , zero
@@ -2440,82 +2504,6 @@ sqlVdbeMemTooBig(Mem * p)
 	return 0;
 }
 
-/*
- * Compare the values contained by the two memory cells, returning
- * negative, zero or positive if pMem1 is less than, equal to, or greater
- * than pMem2. Sorting order is NULL's first, followed by numbers (integers
- * and reals) sorted numerically, followed by text ordered by the collating
- * sequence pColl and finally blob's ordered by memcmp().
- *
- * Two NULL values are considered equal by this function.
- */
-int
-sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
-{
-	int res;
-
-	enum mem_type type1 = pMem1->type;
-	enum mem_type type2 = pMem2->type;
-
-	/* If one value is NULL, it is less than the other. If both values
-	 * are NULL, return 0.
-	 */
-	if (((type1 | type2) & MEM_TYPE_NULL) != 0)
-		return (int)(type2 == MEM_TYPE_NULL) -
-		       (int)(type1 == MEM_TYPE_NULL);
-
-	if (((type1 | type2) & MEM_TYPE_BOOL) != 0) {
-		if ((type1 & type2 & MEM_TYPE_BOOL) != 0) {
-			if (pMem1->u.b == pMem2->u.b)
-				return 0;
-			if (pMem1->u.b)
-				return 1;
-			return -1;
-		}
-		if (type2 == MEM_TYPE_BOOL)
-			return +1;
-		return -1;
-	}
-
-	if (((type1 | type2) & MEM_TYPE_UUID) != 0) {
-		if (mem_cmp_uuid(pMem1, pMem2, &res) == 0)
-			return res;
-		if (type1 != MEM_TYPE_UUID)
-			return +1;
-		return -1;
-	}
-
-	/* At least one of the two values is a number
-	 */
-	if (((type1 | type2) &
-	     (MEM_TYPE_INT | MEM_TYPE_UINT | MEM_TYPE_DOUBLE)) != 0) {
-		if (!mem_is_num(pMem1))
-			return +1;
-		if (!mem_is_num(pMem2))
-			return -1;
-		mem_cmp_num(pMem1, pMem2, &res);
-		return res;
-	}
-
-	/* If one value is a string and the other is a blob, the string is less.
-	 * If both are strings, compare using the collating functions.
-	 */
-	if (((type1 | type2) & MEM_TYPE_STR) != 0) {
-		if (type1 != MEM_TYPE_STR) {
-			return 1;
-		}
-		if (type2 != MEM_TYPE_STR) {
-			return -1;
-		}
-		mem_cmp_str(pMem1, pMem2, &res, pColl);
-		return res;
-	}
-
-	/* Both values must be blobs.  Compare using memcmp().  */
-	mem_cmp_bin(pMem1, pMem2, &res);
-	return res;
-}
-
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index b3cd5c545..bbb99c4d2 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -696,6 +696,14 @@ mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result);
 int
 mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result);
 
+/**
+ * Compare two MEMs using SCALAR rules and return the result of comparison. MEMs
+ * should be scalars. Original MEMs are not changed.
+ */
+int
+mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
+	       const struct coll *coll);
+
 /**
  * Convert the given MEM to INTEGER. This function and the function below define
  * the rules that are used to convert values of all other types to INTEGER. In
@@ -961,8 +969,6 @@ int sqlVdbeMemTooBig(Mem *);
 #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
 			   ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0)
 
-int sqlMemCompare(const Mem *, const Mem *, const struct coll *);
-
 /** MEM manipulate functions. */
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 32d02d96e..220e8b269 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1828,7 +1828,7 @@ case OP_Compare: {
 		assert(i < (int)def->part_count);
 		struct coll *coll = def->parts[i].coll;
 		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
-		iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll);
+		mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll);
 		if (iCompare) {
 			if (is_rev)
 				iCompare = -iCompare;
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e5f35fbf8..dadc6d4a2 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
 			rc = sql_stat4_column(db, samples[i].sample_key, nEq,
 					      &pVal);
 			if (rc == 0 && p1 != NULL) {
-				int res = sqlMemCompare(p1, pVal, coll);
+				int res;
+				mem_cmp_scalar(p1, pVal, &res, coll);
 				if (res >= 0)
 					nLower++;
 			}
 			if (rc == 0 && p2 != NULL) {
-				int res = sqlMemCompare(p2, pVal, coll);
+				int res;
+				mem_cmp_scalar(p2, pVal, &res, coll);
 				if (res >= 0)
 					nUpper++;
 			}
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 4fc5052d8..6b4a811c3 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(4)
+test:plan(8)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -35,4 +35,47 @@ test:do_test(
     end,
     uuid3)
 
+--
+-- Make sure a comparison that includes a UUID and follows the SCALAR rules is
+-- working correctly.
+--
+box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, s SCALAR);]])
+box.execute([[INSERT INTO t VALUES (1, ?)]], {uuid1})
+
+test:do_execsql_test(
+    "gh-6164-5",
+    [[
+        SELECT GREATEST(i, s, x'33', 'something') FROM t;
+    ]], {
+        uuid1
+    })
+
+test:do_execsql_test(
+    "gh-6164-6",
+    [[
+        SELECT LEAST(i, s, x'33', 'something') FROM t;
+    ]], {
+        1
+    })
+
+box.execute([[INSERT INTO t VALUES (2, 2);]])
+
+test:do_execsql_test(
+    "gh-6164-7",
+    [[
+        SELECT MAX(s) FROM t;
+    ]], {
+        uuid1
+    })
+
+test:do_execsql_test(
+    "gh-6164-8",
+    [[
+        SELECT MIN(s) FROM t;
+    ]], {
+        2
+    })
+
+box.execute([[DROP TABLE t;]])
+
 test:finish_test()
-- 
2.25.1


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

* [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches
@ 2021-07-10 14:33 ` Mergen Imeev via Tarantool-patches
  2021-07-11 15:05   ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-15 20:44 ` [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Vladislav Shpilevoy via Tarantool-patches
  4 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch introduces the mem_cmp_scalar() function that compares MEM
and packed to msgpack value using SCALAR rules. MEM and packed value
must be scalars. Prior to this patch, there was a function that used
SCALAR rules to compare MEM and packed value, but its design became
overly complex as new types appeared.

Closes #6164
---
 src/box/sql.c                                 |   9 +-
 src/box/sql/mem.c                             | 282 +++++-------------
 src/box/sql/mem.h                             |  23 +-
 test/sql-tap/gh-6164-uuid-follow-ups.test.lua |  14 +-
 4 files changed, 100 insertions(+), 228 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 790ca7f70..7f24bd778 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -765,13 +765,16 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
 			}
 		}
 		next_fieldno = fieldno + 1;
-		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
+		struct key_part *part = &unpacked->key_def->parts[i];
+		struct Mem *mem = unpacked->aMem + i;
+		struct coll *coll = part->coll;
+		mem_cmp_msgpack(mem, p, &rc, coll);
 		if (rc != 0) {
-			if (unpacked->key_def->parts[i].sort_order !=
-			    SORT_ORDER_ASC)
+			if (part->sort_order == SORT_ORDER_ASC)
 				rc = -rc;
 			goto out;
 		}
+		mp_next(&p);
 	}
 	rc = unpacked->default_rc;
 out:
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 576596c9f..471b69a18 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2073,35 +2073,72 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 	return 0;
 }
 
-/*
- * Both *pMem1 and *pMem2 contain string values. Compare the two values
- * using the collation sequence pColl. As usual, return a negative , zero
- * or positive value if *pMem1 is less than, equal to or greater than
- * *pMem2, respectively. Similar in spirit to "rc = (*pMem1) - (*pMem2);".
- *
- * Strungs assume to be UTF-8 encoded
- */
-static int
-vdbeCompareMemString(const Mem * pMem1, const Mem * pMem2,
-		     const struct coll * pColl)
-{
-	return pColl->cmp(pMem1->z, (size_t)pMem1->n,
-			      pMem2->z, (size_t)pMem2->n, pColl);
-}
-
-/*
- * The input pBlob is guaranteed to be a Blob that is not marked
- * with MEM_Zero.  Return true if it could be a zero-blob.
- */
-static int
-isAllZero(const char *z, int n)
+int
+mem_cmp_msgpack(const struct Mem *a, const char *b, int *result,
+		const struct coll *coll)
 {
-	int i;
-	for (i = 0; i < n; i++) {
-		if (z[i])
-			return 0;
+	struct Mem mem;
+	switch (mp_typeof(*b)) {
+	case MP_NIL:
+		mem.type = MEM_TYPE_NULL;
+		break;
+	case MP_BOOL:
+		mem.type = MEM_TYPE_BOOL;
+		mem.u.b = mp_decode_bool(&b);
+		break;
+	case MP_UINT:
+		mem.type = MEM_TYPE_UINT;
+		mem.u.u = mp_decode_uint(&b);
+		break;
+	case MP_INT:
+		mem.type = MEM_TYPE_INT;
+		mem.u.i = mp_decode_int(&b);
+		break;
+	case MP_FLOAT:
+		mem.type = MEM_TYPE_DOUBLE;
+		mem.u.r = mp_decode_float(&b);
+		break;
+	case MP_DOUBLE:
+		mem.type = MEM_TYPE_DOUBLE;
+		mem.u.r = mp_decode_double(&b);
+		break;
+	case MP_STR:
+		mem.type = MEM_TYPE_STR;
+		mem.n = mp_decode_strl(&b);
+		mem.z = (char *)b;
+		mem.flags = MEM_Ephem;
+		break;
+	case MP_BIN:
+		mem.type = MEM_TYPE_BIN;
+		mem.n = mp_decode_binl(&b);
+		mem.z = (char *)b;
+		break;
+	case MP_ARRAY:
+	case MP_MAP:
+		*result = -1;
+		return 0;
+	case MP_EXT: {
+		int8_t type;
+		const char *buf = b;
+		uint32_t len = mp_decode_extl(&b, &type);
+		if (type == MP_UUID) {
+			assert(len == UUID_LEN);
+			mem.type = MEM_TYPE_UUID;
+			b = buf;
+			if (mp_decode_uuid(&b, &mem.u.uuid) == NULL)
+				return -1;
+			break;
+		}
+		b += len;
+		mem.type = MEM_TYPE_BIN;
+		mem.z = (char *)buf;
+		mem.n = b - buf;
+		break;
 	}
-	return 1;
+	default:
+		unreachable();
+	}
+	return mem_cmp_scalar(a, &mem, result, coll);
 }
 
 char *
@@ -2530,183 +2567,6 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 	return ctx.is_aborted ? -1 : 0;
 }
 
-int
-sqlVdbeCompareMsgpack(const char **key1,
-			  struct UnpackedRecord *unpacked, int key2_idx)
-{
-	const char *aKey1 = *key1;
-	Mem *pKey2 = unpacked->aMem + key2_idx;
-	Mem mem1;
-	int rc = 0;
-	switch (mp_typeof(*aKey1)) {
-	default:{
-			/* FIXME */
-			rc = -1;
-			break;
-		}
-	case MP_NIL:{
-			rc = -(pKey2->type != MEM_TYPE_NULL);
-			mp_decode_nil(&aKey1);
-			break;
-		}
-	case MP_BOOL:{
-			mem1.u.b = mp_decode_bool(&aKey1);
-			if (pKey2->type == MEM_TYPE_BOOL) {
-				if (mem1.u.b != pKey2->u.b)
-					rc = mem1.u.b ? 1 : -1;
-			} else {
-				rc = pKey2->type == MEM_TYPE_NULL ? 1 : -1;
-			}
-			break;
-		}
-	case MP_UINT:{
-			mem1.u.u = mp_decode_uint(&aKey1);
-			if (pKey2->type == MEM_TYPE_INT) {
-				rc = +1;
-			} else if (pKey2->type == MEM_TYPE_UINT) {
-				if (mem1.u.u < pKey2->u.u)
-					rc = -1;
-				else if (mem1.u.u > pKey2->u.u)
-					rc = +1;
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				rc = double_compare_uint64(pKey2->u.r,
-							   mem1.u.u, -1);
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_INT:{
-			mem1.u.i = mp_decode_int(&aKey1);
-			if (pKey2->type == MEM_TYPE_UINT) {
-				rc = -1;
-			} else if (pKey2->type == MEM_TYPE_INT) {
-				if (mem1.u.i < pKey2->u.i) {
-					rc = -1;
-				} else if (mem1.u.i > pKey2->u.i) {
-					rc = +1;
-				}
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				rc = double_compare_nint64(pKey2->u.r, mem1.u.i,
-							   -1);
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_FLOAT:{
-			mem1.u.r = mp_decode_float(&aKey1);
-			goto do_float;
-		}
-	case MP_DOUBLE:{
-			mem1.u.r = mp_decode_double(&aKey1);
- do_float:
-			if (pKey2->type == MEM_TYPE_INT) {
-				rc = double_compare_nint64(mem1.u.r, pKey2->u.i,
-							   1);
-			} else if (pKey2->type == MEM_TYPE_UINT) {
-				rc = double_compare_uint64(mem1.u.r,
-							   pKey2->u.u, 1);
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				if (mem1.u.r < pKey2->u.r) {
-					rc = -1;
-				} else if (mem1.u.r > pKey2->u.r) {
-					rc = +1;
-				}
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_STR:{
-			if (pKey2->type == MEM_TYPE_STR) {
-				struct key_def *key_def = unpacked->key_def;
-				mem1.n = mp_decode_strl(&aKey1);
-				mem1.z = (char *)aKey1;
-				aKey1 += mem1.n;
-				struct coll *coll =
-					key_def->parts[key2_idx].coll;
-				if (coll != NULL) {
-					mem1.type = MEM_TYPE_STR;
-					mem1.flags = 0;
-					rc = vdbeCompareMemString(&mem1, pKey2,
-								  coll);
-				} else {
-					goto do_bin_cmp;
-				}
-			} else {
-				rc = pKey2->type == MEM_TYPE_BIN ? -1 : +1;
-			}
-			break;
-		}
-	case MP_BIN:{
-			mem1.n = mp_decode_binl(&aKey1);
-			mem1.z = (char *)aKey1;
-			aKey1 += mem1.n;
- do_blob:
-			if (pKey2->type == MEM_TYPE_BIN) {
-				if (pKey2->flags & MEM_Zero) {
-					if (!isAllZero
-					    ((const char *)mem1.z, mem1.n)) {
-						rc = 1;
-					} else {
-						rc = mem1.n - pKey2->u.nZero;
-					}
-				} else {
-					int nCmp;
- do_bin_cmp:
-					nCmp = MIN(mem1.n, pKey2->n);
-					rc = memcmp(mem1.z, pKey2->z, nCmp);
-					if (rc == 0)
-						rc = mem1.n - pKey2->n;
-				}
-			} else {
-				rc = 1;
-			}
-			break;
-		}
-	case MP_ARRAY:
-	case MP_MAP: {
-			mem1.z = (char *)aKey1;
-			mp_next(&aKey1);
-			mem1.n = aKey1 - (char *)mem1.z;
-			goto do_blob;
-		}
-	case MP_EXT: {
-		int8_t type;
-		const char *buf = aKey1;
-		uint32_t len = mp_decode_extl(&aKey1, &type);
-		if (type == MP_UUID) {
-			assert(len == UUID_LEN);
-			mem1.type = MEM_TYPE_UUID;
-			aKey1 = buf;
-			if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL ||
-			    mem_cmp_uuid(&mem1, pKey2, &rc) != 0)
-				rc = 1;
-			break;
-		}
-		aKey1 += len;
-		mem1.z = (char *)buf;
-		mem1.n = aKey1 - buf;
-		goto do_blob;
-	}
-	}
-	*key1 = aKey1;
-	return rc;
-}
-
 int
 sqlVdbeRecordCompareMsgpack(const void *key1,
 				struct UnpackedRecord *key2)
@@ -2717,14 +2577,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 	n = MIN(n, key2->nField);
 
 	for (i = 0; i != n; i++) {
-		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
+		struct key_part *part = &key2->key_def->parts[i];
+		struct Mem *mem = key2->aMem + i;
+		struct coll *coll = part->coll;
+		mem_cmp_msgpack(mem, key1, &rc, coll);
 		if (rc != 0) {
-			if (key2->key_def->parts[i].sort_order !=
-			    SORT_ORDER_ASC) {
-				rc = -rc;
-			}
-			return rc;
+			if (part->sort_order != SORT_ORDER_ASC)
+				return rc;
+			return -rc;
 		}
+		mp_next((const char **)&key1);
 	}
 
 	key2->eqSeen = 1;
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index bbb99c4d2..2439fd93b 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -704,6 +704,15 @@ int
 mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 	       const struct coll *coll);
 
+/**
+ * Compare MEM and packed to msgpack value using SCALAR rules and return the
+ * result of comparison. Both values should be scalars. Original MEM and packed
+ * value are not changed.
+ */
+int
+mem_cmp_msgpack(const struct Mem *a, const char *b, int *result,
+		const struct coll *coll);
+
 /**
  * Convert the given MEM to INTEGER. This function and the function below define
  * the rules that are used to convert values of all other types to INTEGER. In
@@ -981,20 +990,6 @@ int sqlVdbeMemTooBig(Mem *);
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func);
 
-/** MEM and msgpack functions. */
-
-/**
- * Perform comparison of two keys: one is packed and one is not.
- *
- * @param key1 Pointer to pointer to first key.
- * @param unpacked Pointer to unpacked tuple.
- * @param key2_idx index of key in umpacked record to compare.
- *
- * @retval +1 if key1 > pUnpacked[iKey2], -1 ptherwise.
- */
-int sqlVdbeCompareMsgpack(const char **key1,
-			      struct UnpackedRecord *unpacked, int key2_idx);
-
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
  *
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 6b4a811c3..dd041c0b4 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(8)
+test:plan(9)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -76,6 +76,18 @@ test:do_execsql_test(
         2
     })
 
+box.execute([[DELETE FROM t;]])
+
+box.execute([[INSERT INTO t VALUES (1, X'00'), (2, ?);]], {uuid1})
+
+test:do_execsql_test(
+    "gh-6164-9",
+    [[
+        SELECT i FROM t ORDER BY s;
+    ]], {
+        1, 2
+    })
+
 box.execute([[DROP TABLE t;]])
 
 test:finish_test()
-- 
2.25.1


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

* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches
@ 2021-07-11 15:03   ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-11 17:51     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-11 15:03 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index aa565277c..dc99bd390 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
>  	for (i = 1; i < argc; i++) {
>  		if (mem_is_null(argv[i]))
>  			return;
> -		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
> -		    0) {
> -			testcase(mask == 0);
> +		int res;
> +		mem_cmp_scalar(argv[iBest], argv[i], &res, pColl);
> +		if ((res ^ mask) >= 0)

1. It seems that under certain conditions if cmp_scalar fails, res remains
not initialized. Which can lead to behaviour changing from run to run. The
same in the other places below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 2595e2fd4..576596c9f 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -59,6 +59,40 @@ enum {
>  	BUF_SIZE = 32,
>  };
>  
> +enum mem_class {
> +	MEM_CLASS_NULL,
> +	MEM_CLASS_BOOL,
> +	MEM_CLASS_NUMBER,
> +	MEM_CLASS_STR,
> +	MEM_CLASS_BIN,
> +	MEM_CLASS_UUID,
> +	mem_class_max,
> +};

2. It might make sense to add a comment that these must be sorted
exactly like enum mp_class.

> +
> +static inline enum mem_class
> +mem_type_class(enum mem_type type)
> +{
> +	switch (type) {
> +	case MEM_TYPE_NULL:
> +		return MEM_CLASS_NULL;
> +	case MEM_TYPE_UINT:
> +	case MEM_TYPE_INT:
> +	case MEM_TYPE_DOUBLE:
> +		return MEM_CLASS_NUMBER;
> +	case MEM_TYPE_STR:
> +		return MEM_CLASS_STR;
> +	case MEM_TYPE_BIN:
> +		return MEM_CLASS_BIN;
> +	case MEM_TYPE_BOOL:
> +		return MEM_CLASS_BOOL;
> +	case MEM_TYPE_UUID:
> +		return MEM_CLASS_UUID;

3. It might work faster without branching if done like
'static enum mp_class mp_classes[]' - would allow to take
the class for any type as simple as an array access
operation.

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
@ 2021-07-11 15:05   ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-11 17:59     ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-11 15:05 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Thanks for the patch!

See 4 comments below.

On 10.07.2021 16:33, Mergen Imeev via Tarantool-patches wrote:
> This patch introduces the mem_cmp_scalar() function that compares MEM

1. scalar -> msgpack.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 576596c9f..471b69a18 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -2073,35 +2073,72 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,

<...>

> -
> -/*
> - * The input pBlob is guaranteed to be a Blob that is not marked
> - * with MEM_Zero.  Return true if it could be a zero-blob.
> - */
> -static int
> -isAllZero(const char *z, int n)
> +int
> +mem_cmp_msgpack(const struct Mem *a, const char *b, int *result,
> +		const struct coll *coll)

2. Maybe better make the function return 'b' after decoding via
making it an out parameter. So as the caller could save mp_next()
call.

>  {
> -	int i;
> -	for (i = 0; i < n; i++) {
> -		if (z[i])
> -			return 0;
> +	struct Mem mem;
> +	switch (mp_typeof(*b)) {
> +	case MP_NIL:
> +		mem.type = MEM_TYPE_NULL;
> +		break;
> +	case MP_BOOL:
> +		mem.type = MEM_TYPE_BOOL;
> +		mem.u.b = mp_decode_bool(&b);
> +		break;
> +	case MP_UINT:
> +		mem.type = MEM_TYPE_UINT;
> +		mem.u.u = mp_decode_uint(&b);
> +		break;
> +	case MP_INT:
> +		mem.type = MEM_TYPE_INT;
> +		mem.u.i = mp_decode_int(&b);
> +		break;
> +	case MP_FLOAT:
> +		mem.type = MEM_TYPE_DOUBLE;
> +		mem.u.r = mp_decode_float(&b);
> +		break;
> +	case MP_DOUBLE:
> +		mem.type = MEM_TYPE_DOUBLE;
> +		mem.u.r = mp_decode_double(&b);
> +		break;
> +	case MP_STR:
> +		mem.type = MEM_TYPE_STR;
> +		mem.n = mp_decode_strl(&b);
> +		mem.z = (char *)b;
> +		mem.flags = MEM_Ephem;
> +		break;
> +	case MP_BIN:
> +		mem.type = MEM_TYPE_BIN;
> +		mem.n = mp_decode_binl(&b);
> +		mem.z = (char *)b;

3. Shouldn't this also have MEM_Ephem?

> +		break;
> +	case MP_ARRAY:
> +	case MP_MAP:
> +		*result = -1;
> +		return 0;
> +	case MP_EXT: {
> +		int8_t type;
> +		const char *buf = b;
> +		uint32_t len = mp_decode_extl(&b, &type);
> +		if (type == MP_UUID) {
> +			assert(len == UUID_LEN);
> +			mem.type = MEM_TYPE_UUID;
> +			b = buf;

4. You can decode &buf below without changing b.

> +			if (mp_decode_uuid(&b, &mem.u.uuid) == NULL)
> +				return -1;
> +			break;
> +		}
> +		b += len;
> +		mem.type = MEM_TYPE_BIN;
> +		mem.z = (char *)buf;
> +		mem.n = b - buf;
> +		break;
>  	}
> -	return 1;
> +	default:
> +		unreachable();
> +	}
> +	return mem_cmp_scalar(a, &mem, result, coll);
>  }

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

* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
  2021-07-11 15:03   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-07-11 17:51     ` Mergen Imeev via Tarantool-patches
  2021-07-12 21:06       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-11 17:51 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Sun, Jul 11, 2021 at 05:03:34PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 3 comments below.
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index aa565277c..dc99bd390 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
> >  	for (i = 1; i < argc; i++) {
> >  		if (mem_is_null(argv[i]))
> >  			return;
> > -		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
> > -		    0) {
> > -			testcase(mask == 0);
> > +		int res;
> > +		mem_cmp_scalar(argv[iBest], argv[i], &res, pColl);
> > +		if ((res ^ mask) >= 0)
> 
> 1. It seems that under certain conditions if cmp_scalar fails, res remains
> not initialized. Which can lead to behaviour changing from run to run. The
> same in the other places below.
> 
Fixed, added checks. Also, I believe there shoudn't be any problem when we
compare two values of the same type. Right now all comparison functions can
return -1 when they were given wrong argument, but I believe I will fix this
when I remove implicit cast. I want them to accept only values of predefined
types and return result of comparison. Then we can get rid of unnecessary
checks.

> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 2595e2fd4..576596c9f 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -59,6 +59,40 @@ enum {
> >  	BUF_SIZE = 32,
> >  };
> >  
> > +enum mem_class {
> > +	MEM_CLASS_NULL,
> > +	MEM_CLASS_BOOL,
> > +	MEM_CLASS_NUMBER,
> > +	MEM_CLASS_STR,
> > +	MEM_CLASS_BIN,
> > +	MEM_CLASS_UUID,
> > +	mem_class_max,
> > +};
> 
> 2. It might make sense to add a comment that these must be sorted
> exactly like enum mp_class.
> 
Added.

> > +
> > +static inline enum mem_class
> > +mem_type_class(enum mem_type type)
> > +{
> > +	switch (type) {
> > +	case MEM_TYPE_NULL:
> > +		return MEM_CLASS_NULL;
> > +	case MEM_TYPE_UINT:
> > +	case MEM_TYPE_INT:
> > +	case MEM_TYPE_DOUBLE:
> > +		return MEM_CLASS_NUMBER;
> > +	case MEM_TYPE_STR:
> > +		return MEM_CLASS_STR;
> > +	case MEM_TYPE_BIN:
> > +		return MEM_CLASS_BIN;
> > +	case MEM_TYPE_BOOL:
> > +		return MEM_CLASS_BOOL;
> > +	case MEM_TYPE_UUID:
> > +		return MEM_CLASS_UUID;
> 
> 3. It might work faster without branching if done like
> 'static enum mp_class mp_classes[]' - would allow to take
> the class for any type as simple as an array access
> operation.
This cannot be done directly in current design since value in enum mem_type are
not sequential numbers. However, this can be done using something like this:

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 4062ff4b3..05a3fb699 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -73,28 +73,27 @@ enum mem_class {
 	mem_class_max,
 };
 
+enum mem_class mem_classes[] = {
+	/** MEM_TYPE_NULL	 = */ MEM_CLASS_NULL,
+	/** MEM_TYPE_UINT	 = */ MEM_CLASS_NUMBER,
+	/** MEM_TYPE_INT	 = */ MEM_CLASS_NUMBER,
+	/** MEM_TYPE_STR	 = */ MEM_CLASS_STR,
+	/** MEM_TYPE_BIN	 = */ MEM_CLASS_BIN,
+	/** MEM_TYPE_ARRAY	 = */ mem_class_max,
+	/** MEM_TYPE_MAP	 = */ mem_class_max,
+	/** MEM_TYPE_BOOL	 = */ MEM_CLASS_BOOL,
+	/** MEM_TYPE_DOUBLE	 = */ MEM_CLASS_NUMBER,
+	/** MEM_TYPE_UUID	 = */ MEM_CLASS_UUID,
+	/** MEM_TYPE_INVALID	 = */ mem_class_max,
+	/** MEM_TYPE_FRAME	 = */ mem_class_max,
+	/** MEM_TYPE_PTR	 = */ mem_class_max,
+	/** MEM_TYPE_AGG	 = */ mem_class_max,
+};
+
 static inline enum mem_class
 mem_type_class(enum mem_type type)
 {
-	switch (type) {
-	case MEM_TYPE_NULL:
-		return MEM_CLASS_NULL;
-	case MEM_TYPE_UINT:
-	case MEM_TYPE_INT:
-	case MEM_TYPE_DOUBLE:
-		return MEM_CLASS_NUMBER;
-	case MEM_TYPE_STR:
-		return MEM_CLASS_STR;
-	case MEM_TYPE_BIN:
-		return MEM_CLASS_BIN;
-	case MEM_TYPE_BOOL:
-		return MEM_CLASS_BOOL;
-	case MEM_TYPE_UUID:
-		return MEM_CLASS_UUID;
-	default:
-		break;
-	}
-	return mem_class_max;
+	return mem_classes[ffs(type) - 1];
 }
 
 bool

We can use macro instead of static inline function. What do you think?

Also we can think of way to solve this problem using bit-wise operations,
but it looks too complicated.



Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index dc99bd390..efb14f23e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -145,7 +145,13 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 		if (mem_is_null(argv[i]))
 			return;
 		int res;
-		mem_cmp_scalar(argv[iBest], argv[i], &res, pColl);
+		if (mem_cmp_scalar(argv[iBest], argv[i], &res, pColl) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(argv[i]),
+				 mem_type_to_str(argv[iBest]));
+			context->is_aborted = true;
+			return;
+		}
 		if ((res ^ mask) >= 0)
 			iBest = i;
 	}
@@ -1057,7 +1063,12 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
 	struct coll *pColl = sqlGetFuncCollSeq(context);
 	UNUSED_PARAMETER(NotUsed);
 	int res;
-	mem_cmp_scalar(argv[0], argv[1], &res, pColl);
+	if (mem_cmp_scalar(argv[0], argv[1], &res, pColl) != 0) {
+		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+			 mem_str(argv[1]), mem_type_to_str(argv[0]));
+		context->is_aborted = true;
+		return;
+	}
 	if (res != 0)
 		sql_result_value(context, argv[0]);
 }
@@ -1827,7 +1838,12 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 		 * comparison is inverted.
 		 */
 		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
-		mem_cmp_scalar(pBest, pArg, &cmp, pColl);
+		if (mem_cmp_scalar(pBest, pArg, &cmp, pColl) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(pArg), mem_type_to_str(pBest));
+			context->is_aborted = true;
+			return;
+		}
 		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
 			mem_copy(pBest, pArg);
 		} else {
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 576596c9f..da27cd191 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -59,6 +59,10 @@ enum {
 	BUF_SIZE = 32,
 };
 
+/**
+ * Analogue of enum mp_class for enum mp_type. The order of the classes must be
+ * the same as in the enum mp_class.
+ */
 enum mem_class {
 	MEM_CLASS_NULL,
 	MEM_CLASS_BOOL,



New patch:


commit c65ccc80c55501035ac6d5ab71b0e30460aee0fd
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jul 10 13:56:39 2021 +0300

    sql: introduce mem_cmp_scalar()
    
    This patch introduces the mem_cmp_scalar() function that compares two
    MEMs using SCALAR rules. MEMs must be scalars. Prior to this patch, there was a
    function that used SCALAR rules to compare two MEMs, but its design became
    overly complex as new types appeared.
    
    Part of #6164

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index aa565277c..efb14f23e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -144,11 +144,16 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 	for (i = 1; i < argc; i++) {
 		if (mem_is_null(argv[i]))
 			return;
-		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
-		    0) {
-			testcase(mask == 0);
-			iBest = i;
+		int res;
+		if (mem_cmp_scalar(argv[iBest], argv[i], &res, pColl) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(argv[i]),
+				 mem_type_to_str(argv[iBest]));
+			context->is_aborted = true;
+			return;
 		}
+		if ((res ^ mask) >= 0)
+			iBest = i;
 	}
 	sql_result_value(context, argv[iBest]);
 }
@@ -1057,9 +1062,15 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
 {
 	struct coll *pColl = sqlGetFuncCollSeq(context);
 	UNUSED_PARAMETER(NotUsed);
-	if (sqlMemCompare(argv[0], argv[1], pColl) != 0) {
-		sql_result_value(context, argv[0]);
+	int res;
+	if (mem_cmp_scalar(argv[0], argv[1], &res, pColl) != 0) {
+		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+			 mem_str(argv[1]), mem_type_to_str(argv[0]));
+		context->is_aborted = true;
+		return;
 	}
+	if (res != 0)
+		sql_result_value(context, argv[0]);
 }
 
 /**
@@ -1827,7 +1838,12 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 		 * comparison is inverted.
 		 */
 		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
-		cmp = sqlMemCompare(pBest, pArg, pColl);
+		if (mem_cmp_scalar(pBest, pArg, &cmp, pColl) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(pArg), mem_type_to_str(pBest));
+			context->is_aborted = true;
+			return;
+		}
 		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
 			mem_copy(pBest, pArg);
 		} else {
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 2595e2fd4..da27cd191 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -59,6 +59,44 @@ enum {
 	BUF_SIZE = 32,
 };
 
+/**
+ * Analogue of enum mp_class for enum mp_type. The order of the classes must be
+ * the same as in the enum mp_class.
+ */
+enum mem_class {
+	MEM_CLASS_NULL,
+	MEM_CLASS_BOOL,
+	MEM_CLASS_NUMBER,
+	MEM_CLASS_STR,
+	MEM_CLASS_BIN,
+	MEM_CLASS_UUID,
+	mem_class_max,
+};
+
+static inline enum mem_class
+mem_type_class(enum mem_type type)
+{
+	switch (type) {
+	case MEM_TYPE_NULL:
+		return MEM_CLASS_NULL;
+	case MEM_TYPE_UINT:
+	case MEM_TYPE_INT:
+	case MEM_TYPE_DOUBLE:
+		return MEM_CLASS_NUMBER;
+	case MEM_TYPE_STR:
+		return MEM_CLASS_STR;
+	case MEM_TYPE_BIN:
+		return MEM_CLASS_BIN;
+	case MEM_TYPE_BOOL:
+		return MEM_CLASS_BOOL;
+	case MEM_TYPE_UUID:
+		return MEM_CLASS_UUID;
+	default:
+		break;
+	}
+	return mem_class_max;
+}
+
 bool
 mem_is_field_compatible(const struct Mem *mem, enum field_type type)
 {
@@ -2009,6 +2047,36 @@ mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result)
 	return 0;
 }
 
+int
+mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
+	       const struct coll *coll)
+{
+	enum mem_class class_a = mem_type_class(a->type);
+	enum mem_class class_b = mem_type_class(b->type);
+	if (class_a != class_b) {
+		*result = class_a - class_b;
+		return 0;
+	}
+	switch (class_a) {
+	case MEM_CLASS_NULL:
+		*result = 0;
+		return 0;
+	case MEM_CLASS_BOOL:
+		return mem_cmp_bool(a, b, result);
+	case MEM_CLASS_NUMBER:
+		return mem_cmp_num(a, b, result);
+	case MEM_CLASS_STR:
+		return mem_cmp_str(a, b, result, coll);
+	case MEM_CLASS_BIN:
+		return mem_cmp_bin(a, b, result);
+	case MEM_CLASS_UUID:
+		return mem_cmp_uuid(a, b, result);
+	default:
+		unreachable();
+	}
+	return 0;
+}
+
 /*
  * Both *pMem1 and *pMem2 contain string values. Compare the two values
  * using the collation sequence pColl. As usual, return a negative , zero
@@ -2440,82 +2508,6 @@ sqlVdbeMemTooBig(Mem * p)
 	return 0;
 }
 
-/*
- * Compare the values contained by the two memory cells, returning
- * negative, zero or positive if pMem1 is less than, equal to, or greater
- * than pMem2. Sorting order is NULL's first, followed by numbers (integers
- * and reals) sorted numerically, followed by text ordered by the collating
- * sequence pColl and finally blob's ordered by memcmp().
- *
- * Two NULL values are considered equal by this function.
- */
-int
-sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
-{
-	int res;
-
-	enum mem_type type1 = pMem1->type;
-	enum mem_type type2 = pMem2->type;
-
-	/* If one value is NULL, it is less than the other. If both values
-	 * are NULL, return 0.
-	 */
-	if (((type1 | type2) & MEM_TYPE_NULL) != 0)
-		return (int)(type2 == MEM_TYPE_NULL) -
-		       (int)(type1 == MEM_TYPE_NULL);
-
-	if (((type1 | type2) & MEM_TYPE_BOOL) != 0) {
-		if ((type1 & type2 & MEM_TYPE_BOOL) != 0) {
-			if (pMem1->u.b == pMem2->u.b)
-				return 0;
-			if (pMem1->u.b)
-				return 1;
-			return -1;
-		}
-		if (type2 == MEM_TYPE_BOOL)
-			return +1;
-		return -1;
-	}
-
-	if (((type1 | type2) & MEM_TYPE_UUID) != 0) {
-		if (mem_cmp_uuid(pMem1, pMem2, &res) == 0)
-			return res;
-		if (type1 != MEM_TYPE_UUID)
-			return +1;
-		return -1;
-	}
-
-	/* At least one of the two values is a number
-	 */
-	if (((type1 | type2) &
-	     (MEM_TYPE_INT | MEM_TYPE_UINT | MEM_TYPE_DOUBLE)) != 0) {
-		if (!mem_is_num(pMem1))
-			return +1;
-		if (!mem_is_num(pMem2))
-			return -1;
-		mem_cmp_num(pMem1, pMem2, &res);
-		return res;
-	}
-
-	/* If one value is a string and the other is a blob, the string is less.
-	 * If both are strings, compare using the collating functions.
-	 */
-	if (((type1 | type2) & MEM_TYPE_STR) != 0) {
-		if (type1 != MEM_TYPE_STR) {
-			return 1;
-		}
-		if (type2 != MEM_TYPE_STR) {
-			return -1;
-		}
-		mem_cmp_str(pMem1, pMem2, &res, pColl);
-		return res;
-	}
-
-	/* Both values must be blobs.  Compare using memcmp().  */
-	mem_cmp_bin(pMem1, pMem2, &res);
-	return res;
-}
-
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index b3cd5c545..bbb99c4d2 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -696,6 +696,14 @@ mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result);
 int
 mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result);
 
+/**
+ * Compare two MEMs using SCALAR rules and return the result of comparison. MEMs
+ * should be scalars. Original MEMs are not changed.
+ */
+int
+mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
+	       const struct coll *coll);
+
 /**
  * Convert the given MEM to INTEGER. This function and the function below define
  * the rules that are used to convert values of all other types to INTEGER. In
@@ -961,8 +969,6 @@ int sqlVdbeMemTooBig(Mem *);
 #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
 			   ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0)
 
-int sqlMemCompare(const Mem *, const Mem *, const struct coll *);
-
 /** MEM manipulate functions. */
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 32d02d96e..220e8b269 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1828,7 +1828,7 @@ case OP_Compare: {
 		assert(i < (int)def->part_count);
 		struct coll *coll = def->parts[i].coll;
 		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
-		iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll);
+		mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll);
 		if (iCompare) {
 			if (is_rev)
 				iCompare = -iCompare;
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e5f35fbf8..dadc6d4a2 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
 			rc = sql_stat4_column(db, samples[i].sample_key, nEq,
 					      &pVal);
 			if (rc == 0 && p1 != NULL) {
-				int res = sqlMemCompare(p1, pVal, coll);
+				int res;
+				mem_cmp_scalar(p1, pVal, &res, coll);
 				if (res >= 0)
 					nLower++;
 			}
 			if (rc == 0 && p2 != NULL) {
-				int res = sqlMemCompare(p2, pVal, coll);
+				int res;
+				mem_cmp_scalar(p2, pVal, &res, coll);
 				if (res >= 0)
 					nUpper++;
 			}
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 4fc5052d8..6b4a811c3 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(4)
+test:plan(8)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -35,4 +35,47 @@ test:do_test(
     end,
     uuid3)
 
+--
+-- Make sure a comparison that includes a UUID and follows the SCALAR rules is
+-- working correctly.
+--
+box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, s SCALAR);]])
+box.execute([[INSERT INTO t VALUES (1, ?)]], {uuid1})
+
+test:do_execsql_test(
+    "gh-6164-5",
+    [[
+        SELECT GREATEST(i, s, x'33', 'something') FROM t;
+    ]], {
+        uuid1
+    })
+
+test:do_execsql_test(
+    "gh-6164-6",
+    [[
+        SELECT LEAST(i, s, x'33', 'something') FROM t;
+    ]], {
+        1
+    })
+
+box.execute([[INSERT INTO t VALUES (2, 2);]])
+
+test:do_execsql_test(
+    "gh-6164-7",
+    [[
+        SELECT MAX(s) FROM t;
+    ]], {
+        uuid1
+    })
+
+test:do_execsql_test(
+    "gh-6164-8",
+    [[
+        SELECT MIN(s) FROM t;
+    ]], {
+        2
+    })
+
+box.execute([[DROP TABLE t;]])
+
 test:finish_test()

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-11 15:05   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-07-11 17:59     ` Mergen Imeev via Tarantool-patches
  2021-07-12 21:09       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-11 17:59 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Sun, Jul 11, 2021 at 05:05:38PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 4 comments below.
> 
> On 10.07.2021 16:33, Mergen Imeev via Tarantool-patches wrote:
> > This patch introduces the mem_cmp_scalar() function that compares MEM
> 
> 1. scalar -> msgpack.
> 
Thanks, fixed.

> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 576596c9f..471b69a18 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -2073,35 +2073,72 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
> 
> <...>
> 
> > -
> > -/*
> > - * The input pBlob is guaranteed to be a Blob that is not marked
> > - * with MEM_Zero.  Return true if it could be a zero-blob.
> > - */
> > -static int
> > -isAllZero(const char *z, int n)
> > +int
> > +mem_cmp_msgpack(const struct Mem *a, const char *b, int *result,
> > +		const struct coll *coll)
> 
> 2. Maybe better make the function return 'b' after decoding via
> making it an out parameter. So as the caller could save mp_next()
> call.
> 
Agree. Done. Also, I could use here mp_decode_str()/mp_decode_bin() instead of
mp_decode_strl()/mp_decode_binl() but decided to do this when I will change
struct Mem according to discussion. Hope, I won't forget about this.

> >  {
> > -	int i;
> > -	for (i = 0; i < n; i++) {
> > -		if (z[i])
> > -			return 0;
> > +	struct Mem mem;
> > +	switch (mp_typeof(*b)) {
> > +	case MP_NIL:
> > +		mem.type = MEM_TYPE_NULL;
> > +		break;
> > +	case MP_BOOL:
> > +		mem.type = MEM_TYPE_BOOL;
> > +		mem.u.b = mp_decode_bool(&b);
> > +		break;
> > +	case MP_UINT:
> > +		mem.type = MEM_TYPE_UINT;
> > +		mem.u.u = mp_decode_uint(&b);
> > +		break;
> > +	case MP_INT:
> > +		mem.type = MEM_TYPE_INT;
> > +		mem.u.i = mp_decode_int(&b);
> > +		break;
> > +	case MP_FLOAT:
> > +		mem.type = MEM_TYPE_DOUBLE;
> > +		mem.u.r = mp_decode_float(&b);
> > +		break;
> > +	case MP_DOUBLE:
> > +		mem.type = MEM_TYPE_DOUBLE;
> > +		mem.u.r = mp_decode_double(&b);
> > +		break;
> > +	case MP_STR:
> > +		mem.type = MEM_TYPE_STR;
> > +		mem.n = mp_decode_strl(&b);
> > +		mem.z = (char *)b;
> > +		mem.flags = MEM_Ephem;
> > +		break;
> > +	case MP_BIN:
> > +		mem.type = MEM_TYPE_BIN;
> > +		mem.n = mp_decode_binl(&b);
> > +		mem.z = (char *)b;
> 
> 3. Shouldn't this also have MEM_Ephem?
> 
Fixed here and below.

> > +		break;
> > +	case MP_ARRAY:
> > +	case MP_MAP:
> > +		*result = -1;
> > +		return 0;
> > +	case MP_EXT: {
> > +		int8_t type;
> > +		const char *buf = b;
> > +		uint32_t len = mp_decode_extl(&b, &type);
> > +		if (type == MP_UUID) {
> > +			assert(len == UUID_LEN);
> > +			mem.type = MEM_TYPE_UUID;
> > +			b = buf;
> 
> 4. You can decode &buf below without changing b.
> 
I changed a bit this part.

> > +			if (mp_decode_uuid(&b, &mem.u.uuid) == NULL)
> > +				return -1;
> > +			break;
> > +		}
> > +		b += len;
> > +		mem.type = MEM_TYPE_BIN;
> > +		mem.z = (char *)buf;
> > +		mem.n = b - buf;
> > +		break;
> >  	}
> > -	return 1;
> > +	default:
> > +		unreachable();
> > +	}
> > +	return mem_cmp_scalar(a, &mem, result, coll);
> >  }


Diff:

diff --git a/src/box/sql.c b/src/box/sql.c
index 7f24bd778..a5afcfabb 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -768,13 +768,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
 		struct key_part *part = &unpacked->key_def->parts[i];
 		struct Mem *mem = unpacked->aMem + i;
 		struct coll *coll = part->coll;
-		mem_cmp_msgpack(mem, p, &rc, coll);
+		mem_cmp_msgpack(mem, &p, &rc, coll);
 		if (rc != 0) {
 			if (part->sort_order == SORT_ORDER_ASC)
 				rc = -rc;
 			goto out;
 		}
-		mp_next(&p);
 	}
 	rc = unpacked->default_rc;
 out:
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 5e7b1f28b..4062ff4b3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2078,65 +2078,71 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 }
 
 int
-mem_cmp_msgpack(const struct Mem *a, const char *b, int *result,
+mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
 		const struct coll *coll)
 {
 	struct Mem mem;
-	switch (mp_typeof(*b)) {
+	switch (mp_typeof(**b)) {
 	case MP_NIL:
 		mem.type = MEM_TYPE_NULL;
+		mp_decode_nil(b);
 		break;
 	case MP_BOOL:
 		mem.type = MEM_TYPE_BOOL;
-		mem.u.b = mp_decode_bool(&b);
+		mem.u.b = mp_decode_bool(b);
 		break;
 	case MP_UINT:
 		mem.type = MEM_TYPE_UINT;
-		mem.u.u = mp_decode_uint(&b);
+		mem.u.u = mp_decode_uint(b);
 		break;
 	case MP_INT:
 		mem.type = MEM_TYPE_INT;
-		mem.u.i = mp_decode_int(&b);
+		mem.u.i = mp_decode_int(b);
 		break;
 	case MP_FLOAT:
 		mem.type = MEM_TYPE_DOUBLE;
-		mem.u.r = mp_decode_float(&b);
+		mem.u.r = mp_decode_float(b);
 		break;
 	case MP_DOUBLE:
 		mem.type = MEM_TYPE_DOUBLE;
-		mem.u.r = mp_decode_double(&b);
+		mem.u.r = mp_decode_double(b);
 		break;
 	case MP_STR:
 		mem.type = MEM_TYPE_STR;
-		mem.n = mp_decode_strl(&b);
-		mem.z = (char *)b;
+		mem.n = mp_decode_strl(b);
+		mem.z = (char *)*b;
+		*b += mem.n;
 		mem.flags = MEM_Ephem;
 		break;
 	case MP_BIN:
 		mem.type = MEM_TYPE_BIN;
-		mem.n = mp_decode_binl(&b);
-		mem.z = (char *)b;
+		mem.n = mp_decode_binl(b);
+		mem.z = (char *)*b;
+		*b += mem.n;
+		mem.flags = MEM_Ephem;
 		break;
 	case MP_ARRAY:
 	case MP_MAP:
+		mp_next(b);
 		*result = -1;
 		return 0;
 	case MP_EXT: {
 		int8_t type;
-		const char *buf = b;
-		uint32_t len = mp_decode_extl(&b, &type);
+		const char *buf = *b;
+		uint32_t len = mp_decode_extl(&buf, &type);
 		if (type == MP_UUID) {
 			assert(len == UUID_LEN);
 			mem.type = MEM_TYPE_UUID;
-			b = buf;
-			if (mp_decode_uuid(&b, &mem.u.uuid) == NULL)
+			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
 				return -1;
 			break;
 		}
-		b += len;
+		len += buf - *b;
 		mem.type = MEM_TYPE_BIN;
-		mem.z = (char *)buf;
-		mem.n = b - buf;
+		mem.z = (char *)*b;
+		mem.n = len;
+		mem.flags = MEM_Ephem;
+		*b += len;
 		break;
 	}
 	default:
@@ -2584,13 +2590,12 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 		struct key_part *part = &key2->key_def->parts[i];
 		struct Mem *mem = key2->aMem + i;
 		struct coll *coll = part->coll;
-		mem_cmp_msgpack(mem, key1, &rc, coll);
+		mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll);
 		if (rc != 0) {
 			if (part->sort_order != SORT_ORDER_ASC)
 				return rc;
 			return -rc;
 		}
-		mp_next((const char **)&key1);
 	}
 
 	key2->eqSeen = 1;
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 2439fd93b..c73536efb 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -706,11 +706,12 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 
 /**
  * Compare MEM and packed to msgpack value using SCALAR rules and return the
- * result of comparison. Both values should be scalars. Original MEM and packed
- * value are not changed.
+ * result of comparison. Both values should be scalars. Original MEM is not
+ * changed. If successful, the second argument will contain the address
+ * following the specified packed value.
  */
 int
-mem_cmp_msgpack(const struct Mem *a, const char *b, int *result,
+mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
 		const struct coll *coll);
 
 /**



New patch:


commit aa422cd2f19813503664e7292dbd01e7b0711a7d
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Jul 9 11:59:34 2021 +0300

    sql: introduce mem_cmp_msgpack()
    
    This patch introduces the mem_cmp_msgpack() function that compares MEM
    and packed to msgpack value using SCALAR rules. MEM and packed value
    must be scalars. Prior to this patch, there was a function that used
    SCALAR rules to compare MEM and packed value, but its design became
    overly complex as new types appeared.
    
    Closes #6164

diff --git a/src/box/sql.c b/src/box/sql.c
index 790ca7f70..a5afcfabb 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -765,10 +765,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
 			}
 		}
 		next_fieldno = fieldno + 1;
-		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
+		struct key_part *part = &unpacked->key_def->parts[i];
+		struct Mem *mem = unpacked->aMem + i;
+		struct coll *coll = part->coll;
+		mem_cmp_msgpack(mem, &p, &rc, coll);
 		if (rc != 0) {
-			if (unpacked->key_def->parts[i].sort_order !=
-			    SORT_ORDER_ASC)
+			if (part->sort_order == SORT_ORDER_ASC)
 				rc = -rc;
 			goto out;
 		}
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index da27cd191..4062ff4b3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2077,35 +2077,78 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 	return 0;
 }
 
-/*
- * Both *pMem1 and *pMem2 contain string values. Compare the two values
- * using the collation sequence pColl. As usual, return a negative , zero
- * or positive value if *pMem1 is less than, equal to or greater than
- * *pMem2, respectively. Similar in spirit to "rc = (*pMem1) - (*pMem2);".
- *
- * Strungs assume to be UTF-8 encoded
- */
-static int
-vdbeCompareMemString(const Mem * pMem1, const Mem * pMem2,
-		     const struct coll * pColl)
-{
-	return pColl->cmp(pMem1->z, (size_t)pMem1->n,
-			      pMem2->z, (size_t)pMem2->n, pColl);
-}
-
-/*
- * The input pBlob is guaranteed to be a Blob that is not marked
- * with MEM_Zero.  Return true if it could be a zero-blob.
- */
-static int
-isAllZero(const char *z, int n)
-{
-	int i;
-	for (i = 0; i < n; i++) {
-		if (z[i])
-			return 0;
+int
+mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
+		const struct coll *coll)
+{
+	struct Mem mem;
+	switch (mp_typeof(**b)) {
+	case MP_NIL:
+		mem.type = MEM_TYPE_NULL;
+		mp_decode_nil(b);
+		break;
+	case MP_BOOL:
+		mem.type = MEM_TYPE_BOOL;
+		mem.u.b = mp_decode_bool(b);
+		break;
+	case MP_UINT:
+		mem.type = MEM_TYPE_UINT;
+		mem.u.u = mp_decode_uint(b);
+		break;
+	case MP_INT:
+		mem.type = MEM_TYPE_INT;
+		mem.u.i = mp_decode_int(b);
+		break;
+	case MP_FLOAT:
+		mem.type = MEM_TYPE_DOUBLE;
+		mem.u.r = mp_decode_float(b);
+		break;
+	case MP_DOUBLE:
+		mem.type = MEM_TYPE_DOUBLE;
+		mem.u.r = mp_decode_double(b);
+		break;
+	case MP_STR:
+		mem.type = MEM_TYPE_STR;
+		mem.n = mp_decode_strl(b);
+		mem.z = (char *)*b;
+		*b += mem.n;
+		mem.flags = MEM_Ephem;
+		break;
+	case MP_BIN:
+		mem.type = MEM_TYPE_BIN;
+		mem.n = mp_decode_binl(b);
+		mem.z = (char *)*b;
+		*b += mem.n;
+		mem.flags = MEM_Ephem;
+		break;
+	case MP_ARRAY:
+	case MP_MAP:
+		mp_next(b);
+		*result = -1;
+		return 0;
+	case MP_EXT: {
+		int8_t type;
+		const char *buf = *b;
+		uint32_t len = mp_decode_extl(&buf, &type);
+		if (type == MP_UUID) {
+			assert(len == UUID_LEN);
+			mem.type = MEM_TYPE_UUID;
+			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
+				return -1;
+			break;
+		}
+		len += buf - *b;
+		mem.type = MEM_TYPE_BIN;
+		mem.z = (char *)*b;
+		mem.n = len;
+		mem.flags = MEM_Ephem;
+		*b += len;
+		break;
 	}
-	return 1;
+	default:
+		unreachable();
+	}
+	return mem_cmp_scalar(a, &mem, result, coll);
 }
 
 char *
@@ -2534,183 +2577,6 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 	return ctx.is_aborted ? -1 : 0;
 }
 
-int
-sqlVdbeCompareMsgpack(const char **key1,
-			  struct UnpackedRecord *unpacked, int key2_idx)
-{
-	const char *aKey1 = *key1;
-	Mem *pKey2 = unpacked->aMem + key2_idx;
-	Mem mem1;
-	int rc = 0;
-	switch (mp_typeof(*aKey1)) {
-	default:{
-			/* FIXME */
-			rc = -1;
-			break;
-		}
-	case MP_NIL:{
-			rc = -(pKey2->type != MEM_TYPE_NULL);
-			mp_decode_nil(&aKey1);
-			break;
-		}
-	case MP_BOOL:{
-			mem1.u.b = mp_decode_bool(&aKey1);
-			if (pKey2->type == MEM_TYPE_BOOL) {
-				if (mem1.u.b != pKey2->u.b)
-					rc = mem1.u.b ? 1 : -1;
-			} else {
-				rc = pKey2->type == MEM_TYPE_NULL ? 1 : -1;
-			}
-			break;
-		}
-	case MP_UINT:{
-			mem1.u.u = mp_decode_uint(&aKey1);
-			if (pKey2->type == MEM_TYPE_INT) {
-				rc = +1;
-			} else if (pKey2->type == MEM_TYPE_UINT) {
-				if (mem1.u.u < pKey2->u.u)
-					rc = -1;
-				else if (mem1.u.u > pKey2->u.u)
-					rc = +1;
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				rc = double_compare_uint64(pKey2->u.r,
-							   mem1.u.u, -1);
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_INT:{
-			mem1.u.i = mp_decode_int(&aKey1);
-			if (pKey2->type == MEM_TYPE_UINT) {
-				rc = -1;
-			} else if (pKey2->type == MEM_TYPE_INT) {
-				if (mem1.u.i < pKey2->u.i) {
-					rc = -1;
-				} else if (mem1.u.i > pKey2->u.i) {
-					rc = +1;
-				}
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				rc = double_compare_nint64(pKey2->u.r, mem1.u.i,
-							   -1);
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_FLOAT:{
-			mem1.u.r = mp_decode_float(&aKey1);
-			goto do_float;
-		}
-	case MP_DOUBLE:{
-			mem1.u.r = mp_decode_double(&aKey1);
- do_float:
-			if (pKey2->type == MEM_TYPE_INT) {
-				rc = double_compare_nint64(mem1.u.r, pKey2->u.i,
-							   1);
-			} else if (pKey2->type == MEM_TYPE_UINT) {
-				rc = double_compare_uint64(mem1.u.r,
-							   pKey2->u.u, 1);
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				if (mem1.u.r < pKey2->u.r) {
-					rc = -1;
-				} else if (mem1.u.r > pKey2->u.r) {
-					rc = +1;
-				}
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_STR:{
-			if (pKey2->type == MEM_TYPE_STR) {
-				struct key_def *key_def = unpacked->key_def;
-				mem1.n = mp_decode_strl(&aKey1);
-				mem1.z = (char *)aKey1;
-				aKey1 += mem1.n;
-				struct coll *coll =
-					key_def->parts[key2_idx].coll;
-				if (coll != NULL) {
-					mem1.type = MEM_TYPE_STR;
-					mem1.flags = 0;
-					rc = vdbeCompareMemString(&mem1, pKey2,
-								  coll);
-				} else {
-					goto do_bin_cmp;
-				}
-			} else {
-				rc = pKey2->type == MEM_TYPE_BIN ? -1 : +1;
-			}
-			break;
-		}
-	case MP_BIN:{
-			mem1.n = mp_decode_binl(&aKey1);
-			mem1.z = (char *)aKey1;
-			aKey1 += mem1.n;
- do_blob:
-			if (pKey2->type == MEM_TYPE_BIN) {
-				if (pKey2->flags & MEM_Zero) {
-					if (!isAllZero
-					    ((const char *)mem1.z, mem1.n)) {
-						rc = 1;
-					} else {
-						rc = mem1.n - pKey2->u.nZero;
-					}
-				} else {
-					int nCmp;
- do_bin_cmp:
-					nCmp = MIN(mem1.n, pKey2->n);
-					rc = memcmp(mem1.z, pKey2->z, nCmp);
-					if (rc == 0)
-						rc = mem1.n - pKey2->n;
-				}
-			} else {
-				rc = 1;
-			}
-			break;
-		}
-	case MP_ARRAY:
-	case MP_MAP: {
-			mem1.z = (char *)aKey1;
-			mp_next(&aKey1);
-			mem1.n = aKey1 - (char *)mem1.z;
-			goto do_blob;
-		}
-	case MP_EXT: {
-		int8_t type;
-		const char *buf = aKey1;
-		uint32_t len = mp_decode_extl(&aKey1, &type);
-		if (type == MP_UUID) {
-			assert(len == UUID_LEN);
-			mem1.type = MEM_TYPE_UUID;
-			aKey1 = buf;
-			if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL ||
-			    mem_cmp_uuid(&mem1, pKey2, &rc) != 0)
-				rc = 1;
-			break;
-		}
-		aKey1 += len;
-		mem1.z = (char *)buf;
-		mem1.n = aKey1 - buf;
-		goto do_blob;
-	}
-	}
-	*key1 = aKey1;
-	return rc;
-}
-
 int
 sqlVdbeRecordCompareMsgpack(const void *key1,
 				struct UnpackedRecord *key2)
@@ -2721,13 +2587,14 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 	n = MIN(n, key2->nField);
 
 	for (i = 0; i != n; i++) {
-		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
+		struct key_part *part = &key2->key_def->parts[i];
+		struct Mem *mem = key2->aMem + i;
+		struct coll *coll = part->coll;
+		mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll);
 		if (rc != 0) {
-			if (key2->key_def->parts[i].sort_order !=
-			    SORT_ORDER_ASC) {
-				rc = -rc;
-			}
-			return rc;
+			if (part->sort_order != SORT_ORDER_ASC)
+				return rc;
+			return -rc;
 		}
 	}
 
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index bbb99c4d2..c73536efb 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -704,6 +704,16 @@ int
 mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 	       const struct coll *coll);
 
+/**
+ * Compare MEM and packed to msgpack value using SCALAR rules and return the
+ * result of comparison. Both values should be scalars. Original MEM is not
+ * changed. If successful, the second argument will contain the address
+ * following the specified packed value.
+ */
+int
+mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
+		const struct coll *coll);
+
 /**
  * Convert the given MEM to INTEGER. This function and the function below define
  * the rules that are used to convert values of all other types to INTEGER. In
@@ -981,20 +991,6 @@ int sqlVdbeMemTooBig(Mem *);
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func);
 
-/** MEM and msgpack functions. */
-
-/**
- * Perform comparison of two keys: one is packed and one is not.
- *
- * @param key1 Pointer to pointer to first key.
- * @param unpacked Pointer to unpacked tuple.
- * @param key2_idx index of key in umpacked record to compare.
- *
- * @retval +1 if key1 > pUnpacked[iKey2], -1 ptherwise.
- */
-int sqlVdbeCompareMsgpack(const char **key1,
-			      struct UnpackedRecord *unpacked, int key2_idx);
-
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
  *
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 6b4a811c3..dd041c0b4 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(8)
+test:plan(9)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -76,6 +76,18 @@ test:do_execsql_test(
         2
     })
 
+box.execute([[DELETE FROM t;]])
+
+box.execute([[INSERT INTO t VALUES (1, X'00'), (2, ?);]], {uuid1})
+
+test:do_execsql_test(
+    "gh-6164-9",
+    [[
+        SELECT i FROM t ORDER BY s;
+    ]], {
+        1, 2
+    })
+
 box.execute([[DROP TABLE t;]])
 
 test:finish_test()

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

* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
  2021-07-11 17:51     ` Mergen Imeev via Tarantool-patches
@ 2021-07-12 21:06       ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-13  8:04         ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-12 21:06 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

>>> +static inline enum mem_class
>>> +mem_type_class(enum mem_type type)
>>> +{
>>> +	switch (type) {
>>> +	case MEM_TYPE_NULL:
>>> +		return MEM_CLASS_NULL;
>>> +	case MEM_TYPE_UINT:
>>> +	case MEM_TYPE_INT:
>>> +	case MEM_TYPE_DOUBLE:
>>> +		return MEM_CLASS_NUMBER;
>>> +	case MEM_TYPE_STR:
>>> +		return MEM_CLASS_STR;
>>> +	case MEM_TYPE_BIN:
>>> +		return MEM_CLASS_BIN;
>>> +	case MEM_TYPE_BOOL:
>>> +		return MEM_CLASS_BOOL;
>>> +	case MEM_TYPE_UUID:
>>> +		return MEM_CLASS_UUID;
>>
>> 3. It might work faster without branching if done like
>> 'static enum mp_class mp_classes[]' - would allow to take
>> the class for any type as simple as an array access
>> operation.
> This cannot be done directly in current design since value in enum mem_type are
> not sequential numbers. However, this can be done using something like this:

Ok, I see now. This is because I asked to make them bit fields
before, shame on me. There is another option - use `32 - bit_ctz_u32()`
to get a number from [1, 13] range and use it as an index in a small
array. The problem here is that this is a bit crazy and I can't be sure
if it works faster than this switch-case on all platforms. Hence, lets
leave it as is now then.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 32d02d96e..220e8b269 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1828,7 +1828,7 @@ case OP_Compare: {
>  		assert(i < (int)def->part_count);
>  		struct coll *coll = def->parts[i].coll;
>  		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
> -		iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll);
> +		mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll);

Why don't you check the result here? AFAIU, previously
sqlMemCompare() never returned something undefined. Now iCompare
might be left garbage leading to unstable behaviour. The same
below.

>  		if (iCompare) {
>  			if (is_rev)
>  				iCompare = -iCompare;
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index e5f35fbf8..dadc6d4a2 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
>  			rc = sql_stat4_column(db, samples[i].sample_key, nEq,
>  					      &pVal);
>  			if (rc == 0 && p1 != NULL) {
> -				int res = sqlMemCompare(p1, pVal, coll);
> +				int res;
> +				mem_cmp_scalar(p1, pVal, &res, coll);
>  				if (res >= 0)
>  					nLower++;
>  			}
>  			if (rc == 0 && p2 != NULL) {
> -				int res = sqlMemCompare(p2, pVal, coll);
> +				int res;
> +				mem_cmp_scalar(p2, pVal, &res, coll);
>  				if (res >= 0)
>  					nUpper++;
>  			}

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-11 17:59     ` Mergen Imeev via Tarantool-patches
@ 2021-07-12 21:09       ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-13  8:10         ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-12 21:09 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

See 2 comments below.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 790ca7f70..a5afcfabb 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -765,10 +765,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
>  			}
>  		}
>  		next_fieldno = fieldno + 1;
> -		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
> +		struct key_part *part = &unpacked->key_def->parts[i];
> +		struct Mem *mem = unpacked->aMem + i;
> +		struct coll *coll = part->coll;
> +		mem_cmp_msgpack(mem, &p, &rc, coll);

1. The same as in the previous patch - you either need to fill
rc out parameter even in case of a fail, or check result of
mem_cmp_msgpack() to keep the results stable. Not depending on
the stack's garbage content.

The same in the other places where it is called.

>  		if (rc != 0) {
> -			if (unpacked->key_def->parts[i].sort_order !=
> -			    SORT_ORDER_ASC)
> +			if (part->sort_order == SORT_ORDER_ASC)
>  				rc = -rc;
>  			goto out;
>  		}
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index da27cd191..4062ff4b3 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -2077,35 +2077,78 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,

<...>

> +	case MP_EXT: {
> +		int8_t type;
> +		const char *buf = *b;
> +		uint32_t len = mp_decode_extl(&buf, &type);
> +		if (type == MP_UUID) {
> +			assert(len == UUID_LEN);
> +			mem.type = MEM_TYPE_UUID;
> +			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
> +				return -1;
> +			break;
> +		}
> +		len += buf - *b;
> +		mem.type = MEM_TYPE_BIN;
> +		mem.z = (char *)*b;
> +		mem.n = len;
> +		mem.flags = MEM_Ephem;
> +		*b += len;
> +		break;

2. I just remembered now, that we have uuid_unpack(). It is better
to use when you already decoded the EXT header. Consider this:

====================
@@ -2129,20 +2129,18 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
 	case MP_EXT: {
 		int8_t type;
 		const char *buf = *b;
-		uint32_t len = mp_decode_extl(&buf, &type);
+		uint32_t len = mp_decode_extl(b, &type);
 		if (type == MP_UUID) {
-			assert(len == UUID_LEN);
-			mem.type = MEM_TYPE_UUID;
-			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
+			if (uuid_unpack(&b, len, &mem.u.uuid) == NULL)
 				return -1;
+			mem.type = MEM_TYPE_UUID;
 			break;
 		}
-		len += buf - *b;
 		mem.type = MEM_TYPE_BIN;
-		mem.z = (char *)*b;
-		mem.n = len;
+		mem.z = (char *)buf;
+		mem.n = b - buf + len;
 		mem.flags = MEM_Ephem;
-		*b += len;
+		b += len;
 		break;
 	}
 	default:
====================

(I didn't test though.)

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

* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
  2021-07-12 21:06       ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-07-13  8:04         ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-13  8:04 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answer, diff and new patch below.

On Mon, Jul 12, 2021 at 11:06:03PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> >>> +static inline enum mem_class
> >>> +mem_type_class(enum mem_type type)
> >>> +{
> >>> +	switch (type) {
> >>> +	case MEM_TYPE_NULL:
> >>> +		return MEM_CLASS_NULL;
> >>> +	case MEM_TYPE_UINT:
> >>> +	case MEM_TYPE_INT:
> >>> +	case MEM_TYPE_DOUBLE:
> >>> +		return MEM_CLASS_NUMBER;
> >>> +	case MEM_TYPE_STR:
> >>> +		return MEM_CLASS_STR;
> >>> +	case MEM_TYPE_BIN:
> >>> +		return MEM_CLASS_BIN;
> >>> +	case MEM_TYPE_BOOL:
> >>> +		return MEM_CLASS_BOOL;
> >>> +	case MEM_TYPE_UUID:
> >>> +		return MEM_CLASS_UUID;
> >>
> >> 3. It might work faster without branching if done like
> >> 'static enum mp_class mp_classes[]' - would allow to take
> >> the class for any type as simple as an array access
> >> operation.
> > This cannot be done directly in current design since value in enum mem_type are
> > not sequential numbers. However, this can be done using something like this:
> 
> Ok, I see now. This is because I asked to make them bit fields
> before, shame on me. There is another option - use `32 - bit_ctz_u32()`
> to get a number from [1, 13] range and use it as an index in a small
> array. The problem here is that this is a bit crazy and I can't be sure
> if it works faster than this switch-case on all platforms. Hence, lets
> leave it as is now then.
> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 32d02d96e..220e8b269 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -1828,7 +1828,7 @@ case OP_Compare: {
> >  		assert(i < (int)def->part_count);
> >  		struct coll *coll = def->parts[i].coll;
> >  		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
> > -		iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll);
> > +		mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll);
> 
> Why don't you check the result here? AFAIU, previously
> sqlMemCompare() never returned something undefined. Now iCompare
> might be left garbage leading to unstable behaviour. The same
> below.
> 
Fixed. Sorry, I didn't think about this last time, I should have fixed this in
all places.

> >  		if (iCompare) {
> >  			if (is_rev)
> >  				iCompare = -iCompare;
> > diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> > index e5f35fbf8..dadc6d4a2 100644
> > --- a/src/box/sql/where.c
> > +++ b/src/box/sql/where.c
> > @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
> >  			rc = sql_stat4_column(db, samples[i].sample_key, nEq,
> >  					      &pVal);
> >  			if (rc == 0 && p1 != NULL) {
> > -				int res = sqlMemCompare(p1, pVal, coll);
> > +				int res;
> > +				mem_cmp_scalar(p1, pVal, &res, coll);
> >  				if (res >= 0)
> >  					nLower++;
> >  			}
> >  			if (rc == 0 && p2 != NULL) {
> > -				int res = sqlMemCompare(p2, pVal, coll);
> > +				int res;
> > +				mem_cmp_scalar(p2, pVal, &res, coll);
> >  				if (res >= 0)
> >  					nUpper++;
> >  			}


Diff:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 220e8b269..d322caef9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1828,7 +1828,13 @@ case OP_Compare: {
 		assert(i < (int)def->part_count);
 		struct coll *coll = def->parts[i].coll;
 		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
-		mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll);
+		struct Mem *a = &aMem[p1+idx];
+		struct Mem *b = &aMem[p2+idx];
+		if (mem_cmp_scalar(a, b, &iCompare, coll) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(b),
+				 mem_type_to_str(a));
+			goto abort_due_to_error;
+		}
 		if (iCompare) {
 			if (is_rev)
 				iCompare = -iCompare;
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index dadc6d4a2..e2eb153fb 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -1273,14 +1273,26 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
 					      &pVal);
 			if (rc == 0 && p1 != NULL) {
 				int res;
-				mem_cmp_scalar(p1, pVal, &res, coll);
-				if (res >= 0)
+				rc = mem_cmp_scalar(p1, pVal, &res, coll);
+				if (rc != 0) {
+					diag_set(ClientError,
+						 ER_SQL_TYPE_MISMATCH,
+						 mem_str(pVal),
+						 mem_type_to_str(p1));
+				}
+				if (rc == 0 && res >= 0)
 					nLower++;
 			}
 			if (rc == 0 && p2 != NULL) {
 				int res;
-				mem_cmp_scalar(p2, pVal, &res, coll);
-				if (res >= 0)
+				rc = mem_cmp_scalar(p2, pVal, &res, coll);
+				if (rc != 0) {
+					diag_set(ClientError,
+						 ER_SQL_TYPE_MISMATCH,
+						 mem_str(pVal),
+						 mem_type_to_str(p2));
+				}
+				if (rc == 0 && res >= 0)
 					nUpper++;
 			}
 		}


New patch:

commit e17481d82926fb47b60eb6aefd2fa2110c1baa09
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jul 10 13:56:39 2021 +0300

    sql: introduce mem_cmp_scalar()
    
    This patch introduces the mem_cmp_scalar() function that compares two
    MEMs using SCALAR rules. MEMs must be scalars. Prior to this patch, there was a
    function that used SCALAR rules to compare two MEMs, but its design became
    overly complex as new types appeared.
    
    Part of #6164

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index aa565277c..efb14f23e 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -144,11 +144,16 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
 	for (i = 1; i < argc; i++) {
 		if (mem_is_null(argv[i]))
 			return;
-		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
-		    0) {
-			testcase(mask == 0);
-			iBest = i;
+		int res;
+		if (mem_cmp_scalar(argv[iBest], argv[i], &res, pColl) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(argv[i]),
+				 mem_type_to_str(argv[iBest]));
+			context->is_aborted = true;
+			return;
 		}
+		if ((res ^ mask) >= 0)
+			iBest = i;
 	}
 	sql_result_value(context, argv[iBest]);
 }
@@ -1057,9 +1062,15 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv)
 {
 	struct coll *pColl = sqlGetFuncCollSeq(context);
 	UNUSED_PARAMETER(NotUsed);
-	if (sqlMemCompare(argv[0], argv[1], pColl) != 0) {
-		sql_result_value(context, argv[0]);
+	int res;
+	if (mem_cmp_scalar(argv[0], argv[1], &res, pColl) != 0) {
+		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+			 mem_str(argv[1]), mem_type_to_str(argv[0]));
+		context->is_aborted = true;
+		return;
 	}
+	if (res != 0)
+		sql_result_value(context, argv[0]);
 }
 
 /**
@@ -1827,7 +1838,12 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 		 * comparison is inverted.
 		 */
 		bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
-		cmp = sqlMemCompare(pBest, pArg, pColl);
+		if (mem_cmp_scalar(pBest, pArg, &cmp, pColl) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(pArg), mem_type_to_str(pBest));
+			context->is_aborted = true;
+			return;
+		}
 		if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
 			mem_copy(pBest, pArg);
 		} else {
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 2595e2fd4..da27cd191 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -59,6 +59,44 @@ enum {
 	BUF_SIZE = 32,
 };
 
+/**
+ * Analogue of enum mp_class for enum mp_type. The order of the classes must be
+ * the same as in the enum mp_class.
+ */
+enum mem_class {
+	MEM_CLASS_NULL,
+	MEM_CLASS_BOOL,
+	MEM_CLASS_NUMBER,
+	MEM_CLASS_STR,
+	MEM_CLASS_BIN,
+	MEM_CLASS_UUID,
+	mem_class_max,
+};
+
+static inline enum mem_class
+mem_type_class(enum mem_type type)
+{
+	switch (type) {
+	case MEM_TYPE_NULL:
+		return MEM_CLASS_NULL;
+	case MEM_TYPE_UINT:
+	case MEM_TYPE_INT:
+	case MEM_TYPE_DOUBLE:
+		return MEM_CLASS_NUMBER;
+	case MEM_TYPE_STR:
+		return MEM_CLASS_STR;
+	case MEM_TYPE_BIN:
+		return MEM_CLASS_BIN;
+	case MEM_TYPE_BOOL:
+		return MEM_CLASS_BOOL;
+	case MEM_TYPE_UUID:
+		return MEM_CLASS_UUID;
+	default:
+		break;
+	}
+	return mem_class_max;
+}
+
 bool
 mem_is_field_compatible(const struct Mem *mem, enum field_type type)
 {
@@ -2009,6 +2047,36 @@ mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result)
 	return 0;
 }
 
+int
+mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
+	       const struct coll *coll)
+{
+	enum mem_class class_a = mem_type_class(a->type);
+	enum mem_class class_b = mem_type_class(b->type);
+	if (class_a != class_b) {
+		*result = class_a - class_b;
+		return 0;
+	}
+	switch (class_a) {
+	case MEM_CLASS_NULL:
+		*result = 0;
+		return 0;
+	case MEM_CLASS_BOOL:
+		return mem_cmp_bool(a, b, result);
+	case MEM_CLASS_NUMBER:
+		return mem_cmp_num(a, b, result);
+	case MEM_CLASS_STR:
+		return mem_cmp_str(a, b, result, coll);
+	case MEM_CLASS_BIN:
+		return mem_cmp_bin(a, b, result);
+	case MEM_CLASS_UUID:
+		return mem_cmp_uuid(a, b, result);
+	default:
+		unreachable();
+	}
+	return 0;
+}
+
 /*
  * Both *pMem1 and *pMem2 contain string values. Compare the two values
  * using the collation sequence pColl. As usual, return a negative , zero
@@ -2440,82 +2508,6 @@ sqlVdbeMemTooBig(Mem * p)
 	return 0;
 }
 
-/*
- * Compare the values contained by the two memory cells, returning
- * negative, zero or positive if pMem1 is less than, equal to, or greater
- * than pMem2. Sorting order is NULL's first, followed by numbers (integers
- * and reals) sorted numerically, followed by text ordered by the collating
- * sequence pColl and finally blob's ordered by memcmp().
- *
- * Two NULL values are considered equal by this function.
- */
-int
-sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
-{
-	int res;
-
-	enum mem_type type1 = pMem1->type;
-	enum mem_type type2 = pMem2->type;
-
-	/* If one value is NULL, it is less than the other. If both values
-	 * are NULL, return 0.
-	 */
-	if (((type1 | type2) & MEM_TYPE_NULL) != 0)
-		return (int)(type2 == MEM_TYPE_NULL) -
-		       (int)(type1 == MEM_TYPE_NULL);
-
-	if (((type1 | type2) & MEM_TYPE_BOOL) != 0) {
-		if ((type1 & type2 & MEM_TYPE_BOOL) != 0) {
-			if (pMem1->u.b == pMem2->u.b)
-				return 0;
-			if (pMem1->u.b)
-				return 1;
-			return -1;
-		}
-		if (type2 == MEM_TYPE_BOOL)
-			return +1;
-		return -1;
-	}
-
-	if (((type1 | type2) & MEM_TYPE_UUID) != 0) {
-		if (mem_cmp_uuid(pMem1, pMem2, &res) == 0)
-			return res;
-		if (type1 != MEM_TYPE_UUID)
-			return +1;
-		return -1;
-	}
-
-	/* At least one of the two values is a number
-	 */
-	if (((type1 | type2) &
-	     (MEM_TYPE_INT | MEM_TYPE_UINT | MEM_TYPE_DOUBLE)) != 0) {
-		if (!mem_is_num(pMem1))
-			return +1;
-		if (!mem_is_num(pMem2))
-			return -1;
-		mem_cmp_num(pMem1, pMem2, &res);
-		return res;
-	}
-
-	/* If one value is a string and the other is a blob, the string is less.
-	 * If both are strings, compare using the collating functions.
-	 */
-	if (((type1 | type2) & MEM_TYPE_STR) != 0) {
-		if (type1 != MEM_TYPE_STR) {
-			return 1;
-		}
-		if (type2 != MEM_TYPE_STR) {
-			return -1;
-		}
-		mem_cmp_str(pMem1, pMem2, &res, pColl);
-		return res;
-	}
-
-	/* Both values must be blobs.  Compare using memcmp().  */
-	mem_cmp_bin(pMem1, pMem2, &res);
-	return res;
-}
-
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index b3cd5c545..bbb99c4d2 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -696,6 +696,14 @@ mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result);
 int
 mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result);
 
+/**
+ * Compare two MEMs using SCALAR rules and return the result of comparison. MEMs
+ * should be scalars. Original MEMs are not changed.
+ */
+int
+mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
+	       const struct coll *coll);
+
 /**
  * Convert the given MEM to INTEGER. This function and the function below define
  * the rules that are used to convert values of all other types to INTEGER. In
@@ -961,8 +969,6 @@ int sqlVdbeMemTooBig(Mem *);
 #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
 			   ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0)
 
-int sqlMemCompare(const Mem *, const Mem *, const struct coll *);
-
 /** MEM manipulate functions. */
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 32d02d96e..d322caef9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1828,7 +1828,13 @@ case OP_Compare: {
 		assert(i < (int)def->part_count);
 		struct coll *coll = def->parts[i].coll;
 		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
-		iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll);
+		struct Mem *a = &aMem[p1+idx];
+		struct Mem *b = &aMem[p2+idx];
+		if (mem_cmp_scalar(a, b, &iCompare, coll) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(b),
+				 mem_type_to_str(a));
+			goto abort_due_to_error;
+		}
 		if (iCompare) {
 			if (is_rev)
 				iCompare = -iCompare;
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e5f35fbf8..e2eb153fb 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -1272,13 +1272,27 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
 			rc = sql_stat4_column(db, samples[i].sample_key, nEq,
 					      &pVal);
 			if (rc == 0 && p1 != NULL) {
-				int res = sqlMemCompare(p1, pVal, coll);
-				if (res >= 0)
+				int res;
+				rc = mem_cmp_scalar(p1, pVal, &res, coll);
+				if (rc != 0) {
+					diag_set(ClientError,
+						 ER_SQL_TYPE_MISMATCH,
+						 mem_str(pVal),
+						 mem_type_to_str(p1));
+				}
+				if (rc == 0 && res >= 0)
 					nLower++;
 			}
 			if (rc == 0 && p2 != NULL) {
-				int res = sqlMemCompare(p2, pVal, coll);
-				if (res >= 0)
+				int res;
+				rc = mem_cmp_scalar(p2, pVal, &res, coll);
+				if (rc != 0) {
+					diag_set(ClientError,
+						 ER_SQL_TYPE_MISMATCH,
+						 mem_str(pVal),
+						 mem_type_to_str(p2));
+				}
+				if (rc == 0 && res >= 0)
 					nUpper++;
 			}
 		}
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 4fc5052d8..6b4a811c3 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(4)
+test:plan(8)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -35,4 +35,47 @@ test:do_test(
     end,
     uuid3)
 
+--
+-- Make sure a comparison that includes a UUID and follows the SCALAR rules is
+-- working correctly.
+--
+box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, s SCALAR);]])
+box.execute([[INSERT INTO t VALUES (1, ?)]], {uuid1})
+
+test:do_execsql_test(
+    "gh-6164-5",
+    [[
+        SELECT GREATEST(i, s, x'33', 'something') FROM t;
+    ]], {
+        uuid1
+    })
+
+test:do_execsql_test(
+    "gh-6164-6",
+    [[
+        SELECT LEAST(i, s, x'33', 'something') FROM t;
+    ]], {
+        1
+    })
+
+box.execute([[INSERT INTO t VALUES (2, 2);]])
+
+test:do_execsql_test(
+    "gh-6164-7",
+    [[
+        SELECT MAX(s) FROM t;
+    ]], {
+        uuid1
+    })
+
+test:do_execsql_test(
+    "gh-6164-8",
+    [[
+        SELECT MIN(s) FROM t;
+    ]], {
+        2
+    })
+
+box.execute([[DROP TABLE t;]])
+
 test:finish_test()

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-12 21:09       ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-07-13  8:10         ` Mergen Imeev via Tarantool-patches
  2021-07-13 20:39           ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-13  8:10 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Mon, Jul 12, 2021 at 11:09:02PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/sql.c b/src/box/sql.c
> > index 790ca7f70..a5afcfabb 100644
> > --- a/src/box/sql.c
> > +++ b/src/box/sql.c
> > @@ -765,10 +765,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
> >  			}
> >  		}
> >  		next_fieldno = fieldno + 1;
> > -		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
> > +		struct key_part *part = &unpacked->key_def->parts[i];
> > +		struct Mem *mem = unpacked->aMem + i;
> > +		struct coll *coll = part->coll;
> > +		mem_cmp_msgpack(mem, &p, &rc, coll);
> 
> 1. The same as in the previous patch - you either need to fill
> rc out parameter even in case of a fail, or check result of
> mem_cmp_msgpack() to keep the results stable. Not depending on
> the stack's garbage content.
> 
> The same in the other places where it is called.
> 
Fixed. However, since there is no proper way to throw an error from both
functions where mem_cmp_msgpack() was used, I just made rc equal to 0.

> >  		if (rc != 0) {
> > -			if (unpacked->key_def->parts[i].sort_order !=
> > -			    SORT_ORDER_ASC)
> > +			if (part->sort_order == SORT_ORDER_ASC)
> >  				rc = -rc;
> >  			goto out;
> >  		}
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index da27cd191..4062ff4b3 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -2077,35 +2077,78 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
> 
> <...>
> 
> > +	case MP_EXT: {
> > +		int8_t type;
> > +		const char *buf = *b;
> > +		uint32_t len = mp_decode_extl(&buf, &type);
> > +		if (type == MP_UUID) {
> > +			assert(len == UUID_LEN);
> > +			mem.type = MEM_TYPE_UUID;
> > +			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
> > +				return -1;
> > +			break;
> > +		}
> > +		len += buf - *b;
> > +		mem.type = MEM_TYPE_BIN;
> > +		mem.z = (char *)*b;
> > +		mem.n = len;
> > +		mem.flags = MEM_Ephem;
> > +		*b += len;
> > +		break;
> 
> 2. I just remembered now, that we have uuid_unpack(). It is better
> to use when you already decoded the EXT header. Consider this:
> 
> ====================
> @@ -2129,20 +2129,18 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
>  	case MP_EXT: {
>  		int8_t type;
>  		const char *buf = *b;
> -		uint32_t len = mp_decode_extl(&buf, &type);
> +		uint32_t len = mp_decode_extl(b, &type);
>  		if (type == MP_UUID) {
> -			assert(len == UUID_LEN);
> -			mem.type = MEM_TYPE_UUID;
> -			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
> +			if (uuid_unpack(&b, len, &mem.u.uuid) == NULL)
>  				return -1;
> +			mem.type = MEM_TYPE_UUID;
>  			break;
>  		}
> -		len += buf - *b;
>  		mem.type = MEM_TYPE_BIN;
> -		mem.z = (char *)*b;
> -		mem.n = len;
> +		mem.z = (char *)buf;
> +		mem.n = b - buf + len;
>  		mem.flags = MEM_Ephem;
> -		*b += len;
> +		b += len;
>  		break;
>  	}
>  	default:
> ====================
> 
> (I didn't test though.)
Thank you for the suggestion! I applied your diff with some changes.

Diff:


diff --git a/src/box/sql.c b/src/box/sql.c
index a5afcfabb..c1271ee0a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -768,7 +768,9 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
 		struct key_part *part = &unpacked->key_def->parts[i];
 		struct Mem *mem = unpacked->aMem + i;
 		struct coll *coll = part->coll;
-		mem_cmp_msgpack(mem, &p, &rc, coll);
+		/* In case of fail make rc equal to 0. */
+		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
+			rc = 0;
 		if (rc != 0) {
 			if (part->sort_order == SORT_ORDER_ASC)
 				rc = -rc;
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 4062ff4b3..8e176e418 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2129,20 +2129,19 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
 	case MP_EXT: {
 		int8_t type;
 		const char *buf = *b;
-		uint32_t len = mp_decode_extl(&buf, &type);
+		uint32_t len = mp_decode_extl(b, &type);
 		if (type == MP_UUID) {
 			assert(len == UUID_LEN);
 			mem.type = MEM_TYPE_UUID;
-			if (mp_decode_uuid(b, &mem.u.uuid) == NULL)
+			if (uuid_unpack(b, len, &mem.u.uuid) == NULL)
 				return -1;
 			break;
 		}
-		len += buf - *b;
+		*b += len;
 		mem.type = MEM_TYPE_BIN;
-		mem.z = (char *)*b;
-		mem.n = len;
+		mem.z = (char *)buf;
+		mem.n = *b - buf;
 		mem.flags = MEM_Ephem;
-		*b += len;
 		break;
 	}
 	default:
@@ -2590,7 +2589,9 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 		struct key_part *part = &key2->key_def->parts[i];
 		struct Mem *mem = key2->aMem + i;
 		struct coll *coll = part->coll;
-		mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll);
+		/* In case of fail make rc equal to 0. */
+		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
+			rc = 0;
 		if (rc != 0) {
 			if (part->sort_order != SORT_ORDER_ASC)
 				return rc;


New patch:


commit 2f80030b9914256bf931c95b8d76113f31c01655
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Jul 9 11:59:34 2021 +0300

    sql: introduce mem_cmp_msgpack()
    
    This patch introduces the mem_cmp_msgpack() function that compares MEM
    and packed to msgpack value using SCALAR rules. MEM and packed value
    must be scalars. Prior to this patch, there was a function that used
    SCALAR rules to compare MEM and packed value, but its design became
    overly complex as new types appeared.
    
    Closes #6164

diff --git a/src/box/sql.c b/src/box/sql.c
index 790ca7f70..c1271ee0a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -765,10 +765,14 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
 			}
 		}
 		next_fieldno = fieldno + 1;
-		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
+		struct key_part *part = &unpacked->key_def->parts[i];
+		struct Mem *mem = unpacked->aMem + i;
+		struct coll *coll = part->coll;
+		/* In case of fail make rc equal to 0. */
+		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
+			rc = 0;
 		if (rc != 0) {
-			if (unpacked->key_def->parts[i].sort_order !=
-			    SORT_ORDER_ASC)
+			if (part->sort_order == SORT_ORDER_ASC)
 				rc = -rc;
 			goto out;
 		}
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index da27cd191..8e176e418 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2077,35 +2077,77 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 	return 0;
 }
 
-/*
- * Both *pMem1 and *pMem2 contain string values. Compare the two values
- * using the collation sequence pColl. As usual, return a negative , zero
- * or positive value if *pMem1 is less than, equal to or greater than
- * *pMem2, respectively. Similar in spirit to "rc = (*pMem1) - (*pMem2);".
- *
- * Strungs assume to be UTF-8 encoded
- */
-static int
-vdbeCompareMemString(const Mem * pMem1, const Mem * pMem2,
-		     const struct coll * pColl)
-{
-	return pColl->cmp(pMem1->z, (size_t)pMem1->n,
-			      pMem2->z, (size_t)pMem2->n, pColl);
-}
-
-/*
- * The input pBlob is guaranteed to be a Blob that is not marked
- * with MEM_Zero.  Return true if it could be a zero-blob.
- */
-static int
-isAllZero(const char *z, int n)
-{
-	int i;
-	for (i = 0; i < n; i++) {
-		if (z[i])
-			return 0;
+int
+mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
+		const struct coll *coll)
+{
+	struct Mem mem;
+	switch (mp_typeof(**b)) {
+	case MP_NIL:
+		mem.type = MEM_TYPE_NULL;
+		mp_decode_nil(b);
+		break;
+	case MP_BOOL:
+		mem.type = MEM_TYPE_BOOL;
+		mem.u.b = mp_decode_bool(b);
+		break;
+	case MP_UINT:
+		mem.type = MEM_TYPE_UINT;
+		mem.u.u = mp_decode_uint(b);
+		break;
+	case MP_INT:
+		mem.type = MEM_TYPE_INT;
+		mem.u.i = mp_decode_int(b);
+		break;
+	case MP_FLOAT:
+		mem.type = MEM_TYPE_DOUBLE;
+		mem.u.r = mp_decode_float(b);
+		break;
+	case MP_DOUBLE:
+		mem.type = MEM_TYPE_DOUBLE;
+		mem.u.r = mp_decode_double(b);
+		break;
+	case MP_STR:
+		mem.type = MEM_TYPE_STR;
+		mem.n = mp_decode_strl(b);
+		mem.z = (char *)*b;
+		*b += mem.n;
+		mem.flags = MEM_Ephem;
+		break;
+	case MP_BIN:
+		mem.type = MEM_TYPE_BIN;
+		mem.n = mp_decode_binl(b);
+		mem.z = (char *)*b;
+		*b += mem.n;
+		mem.flags = MEM_Ephem;
+		break;
+	case MP_ARRAY:
+	case MP_MAP:
+		mp_next(b);
+		*result = -1;
+		return 0;
+	case MP_EXT: {
+		int8_t type;
+		const char *buf = *b;
+		uint32_t len = mp_decode_extl(b, &type);
+		if (type == MP_UUID) {
+			assert(len == UUID_LEN);
+			mem.type = MEM_TYPE_UUID;
+			if (uuid_unpack(b, len, &mem.u.uuid) == NULL)
+				return -1;
+			break;
+		}
+		*b += len;
+		mem.type = MEM_TYPE_BIN;
+		mem.z = (char *)buf;
+		mem.n = *b - buf;
+		mem.flags = MEM_Ephem;
+		break;
 	}
-	return 1;
+	default:
+		unreachable();
+	}
+	return mem_cmp_scalar(a, &mem, result, coll);
 }
 
 char *
@@ -2534,183 +2576,6 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 	return ctx.is_aborted ? -1 : 0;
 }
 
-int
-sqlVdbeCompareMsgpack(const char **key1,
-			  struct UnpackedRecord *unpacked, int key2_idx)
-{
-	const char *aKey1 = *key1;
-	Mem *pKey2 = unpacked->aMem + key2_idx;
-	Mem mem1;
-	int rc = 0;
-	switch (mp_typeof(*aKey1)) {
-	default:{
-			/* FIXME */
-			rc = -1;
-			break;
-		}
-	case MP_NIL:{
-			rc = -(pKey2->type != MEM_TYPE_NULL);
-			mp_decode_nil(&aKey1);
-			break;
-		}
-	case MP_BOOL:{
-			mem1.u.b = mp_decode_bool(&aKey1);
-			if (pKey2->type == MEM_TYPE_BOOL) {
-				if (mem1.u.b != pKey2->u.b)
-					rc = mem1.u.b ? 1 : -1;
-			} else {
-				rc = pKey2->type == MEM_TYPE_NULL ? 1 : -1;
-			}
-			break;
-		}
-	case MP_UINT:{
-			mem1.u.u = mp_decode_uint(&aKey1);
-			if (pKey2->type == MEM_TYPE_INT) {
-				rc = +1;
-			} else if (pKey2->type == MEM_TYPE_UINT) {
-				if (mem1.u.u < pKey2->u.u)
-					rc = -1;
-				else if (mem1.u.u > pKey2->u.u)
-					rc = +1;
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				rc = double_compare_uint64(pKey2->u.r,
-							   mem1.u.u, -1);
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_INT:{
-			mem1.u.i = mp_decode_int(&aKey1);
-			if (pKey2->type == MEM_TYPE_UINT) {
-				rc = -1;
-			} else if (pKey2->type == MEM_TYPE_INT) {
-				if (mem1.u.i < pKey2->u.i) {
-					rc = -1;
-				} else if (mem1.u.i > pKey2->u.i) {
-					rc = +1;
-				}
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				rc = double_compare_nint64(pKey2->u.r, mem1.u.i,
-							   -1);
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_FLOAT:{
-			mem1.u.r = mp_decode_float(&aKey1);
-			goto do_float;
-		}
-	case MP_DOUBLE:{
-			mem1.u.r = mp_decode_double(&aKey1);
- do_float:
-			if (pKey2->type == MEM_TYPE_INT) {
-				rc = double_compare_nint64(mem1.u.r, pKey2->u.i,
-							   1);
-			} else if (pKey2->type == MEM_TYPE_UINT) {
-				rc = double_compare_uint64(mem1.u.r,
-							   pKey2->u.u, 1);
-			} else if (pKey2->type == MEM_TYPE_DOUBLE) {
-				if (mem1.u.r < pKey2->u.r) {
-					rc = -1;
-				} else if (mem1.u.r > pKey2->u.r) {
-					rc = +1;
-				}
-			} else if (pKey2->type == MEM_TYPE_NULL) {
-				rc = 1;
-			} else if (pKey2->type == MEM_TYPE_BOOL) {
-				rc = 1;
-			} else {
-				rc = -1;
-			}
-			break;
-		}
-	case MP_STR:{
-			if (pKey2->type == MEM_TYPE_STR) {
-				struct key_def *key_def = unpacked->key_def;
-				mem1.n = mp_decode_strl(&aKey1);
-				mem1.z = (char *)aKey1;
-				aKey1 += mem1.n;
-				struct coll *coll =
-					key_def->parts[key2_idx].coll;
-				if (coll != NULL) {
-					mem1.type = MEM_TYPE_STR;
-					mem1.flags = 0;
-					rc = vdbeCompareMemString(&mem1, pKey2,
-								  coll);
-				} else {
-					goto do_bin_cmp;
-				}
-			} else {
-				rc = pKey2->type == MEM_TYPE_BIN ? -1 : +1;
-			}
-			break;
-		}
-	case MP_BIN:{
-			mem1.n = mp_decode_binl(&aKey1);
-			mem1.z = (char *)aKey1;
-			aKey1 += mem1.n;
- do_blob:
-			if (pKey2->type == MEM_TYPE_BIN) {
-				if (pKey2->flags & MEM_Zero) {
-					if (!isAllZero
-					    ((const char *)mem1.z, mem1.n)) {
-						rc = 1;
-					} else {
-						rc = mem1.n - pKey2->u.nZero;
-					}
-				} else {
-					int nCmp;
- do_bin_cmp:
-					nCmp = MIN(mem1.n, pKey2->n);
-					rc = memcmp(mem1.z, pKey2->z, nCmp);
-					if (rc == 0)
-						rc = mem1.n - pKey2->n;
-				}
-			} else {
-				rc = 1;
-			}
-			break;
-		}
-	case MP_ARRAY:
-	case MP_MAP: {
-			mem1.z = (char *)aKey1;
-			mp_next(&aKey1);
-			mem1.n = aKey1 - (char *)mem1.z;
-			goto do_blob;
-		}
-	case MP_EXT: {
-		int8_t type;
-		const char *buf = aKey1;
-		uint32_t len = mp_decode_extl(&aKey1, &type);
-		if (type == MP_UUID) {
-			assert(len == UUID_LEN);
-			mem1.type = MEM_TYPE_UUID;
-			aKey1 = buf;
-			if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL ||
-			    mem_cmp_uuid(&mem1, pKey2, &rc) != 0)
-				rc = 1;
-			break;
-		}
-		aKey1 += len;
-		mem1.z = (char *)buf;
-		mem1.n = aKey1 - buf;
-		goto do_blob;
-	}
-	}
-	*key1 = aKey1;
-	return rc;
-}
-
 int
 sqlVdbeRecordCompareMsgpack(const void *key1,
 				struct UnpackedRecord *key2)
@@ -2721,13 +2586,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 	n = MIN(n, key2->nField);
 
 	for (i = 0; i != n; i++) {
-		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
+		struct key_part *part = &key2->key_def->parts[i];
+		struct Mem *mem = key2->aMem + i;
+		struct coll *coll = part->coll;
+		/* In case of fail make rc equal to 0. */
+		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
+			rc = 0;
 		if (rc != 0) {
-			if (key2->key_def->parts[i].sort_order !=
-			    SORT_ORDER_ASC) {
-				rc = -rc;
-			}
-			return rc;
+			if (part->sort_order != SORT_ORDER_ASC)
+				return rc;
+			return -rc;
 		}
 	}
 
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index bbb99c4d2..c73536efb 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -704,6 +704,16 @@ int
 mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result,
 	       const struct coll *coll);
 
+/**
+ * Compare MEM and packed to msgpack value using SCALAR rules and return the
+ * result of comparison. Both values should be scalars. Original MEM is not
+ * changed. If successful, the second argument will contain the address
+ * following the specified packed value.
+ */
+int
+mem_cmp_msgpack(const struct Mem *a, const char **b, int *result,
+		const struct coll *coll);
+
 /**
  * Convert the given MEM to INTEGER. This function and the function below define
  * the rules that are used to convert values of all other types to INTEGER. In
@@ -981,20 +991,6 @@ int sqlVdbeMemTooBig(Mem *);
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func);
 
-/** MEM and msgpack functions. */
-
-/**
- * Perform comparison of two keys: one is packed and one is not.
- *
- * @param key1 Pointer to pointer to first key.
- * @param unpacked Pointer to unpacked tuple.
- * @param key2_idx index of key in umpacked record to compare.
- *
- * @retval +1 if key1 > pUnpacked[iKey2], -1 ptherwise.
- */
-int sqlVdbeCompareMsgpack(const char **key1,
-			      struct UnpackedRecord *unpacked, int key2_idx);
-
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
  *
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index 6b4a811c3..dd041c0b4 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(8)
+test:plan(9)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -76,6 +76,18 @@ test:do_execsql_test(
         2
     })
 
+box.execute([[DELETE FROM t;]])
+
+box.execute([[INSERT INTO t VALUES (1, X'00'), (2, ?);]], {uuid1})
+
+test:do_execsql_test(
+    "gh-6164-9",
+    [[
+        SELECT i FROM t ORDER BY s;
+    ]], {
+        1, 2
+    })
+
 box.execute([[DROP TABLE t;]])
 
 test:finish_test()

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-13  8:10         ` Mergen Imeev via Tarantool-patches
@ 2021-07-13 20:39           ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-14  6:51             ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-13 20:39 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Thanks for the fixes!

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 790ca7f70..c1271ee0a 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -765,10 +765,14 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
>  			}
>  		}
>  		next_fieldno = fieldno + 1;
> -		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
> +		struct key_part *part = &unpacked->key_def->parts[i];
> +		struct Mem *mem = unpacked->aMem + i;
> +		struct coll *coll = part->coll;
> +		/* In case of fail make rc equal to 0. */

The comment isn't really necessary I would say. Does not
help much. The same in the other place below.

> +		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
> +			rc = 0;
>  		if (rc != 0) {
> -			if (unpacked->key_def->parts[i].sort_order !=
> -			    SORT_ORDER_ASC)
> +			if (part->sort_order == SORT_ORDER_ASC)

Why did you change != to ==?

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index da27cd191..8e176e418 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -2721,13 +2586,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
>  	n = MIN(n, key2->nField);
>  
>  	for (i = 0; i != n; i++) {
> -		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
> +		struct key_part *part = &key2->key_def->parts[i];
> +		struct Mem *mem = key2->aMem + i;
> +		struct coll *coll = part->coll;
> +		/* In case of fail make rc equal to 0. */
> +		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
> +			rc = 0;

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-13 20:39           ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-07-14  6:51             ` Mergen Imeev via Tarantool-patches
  2021-07-14 21:53               ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-14  6:51 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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

On Tue, Jul 13, 2021 at 10:39:04PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > diff --git a/src/box/sql.c b/src/box/sql.c
> > index 790ca7f70..c1271ee0a 100644
> > --- a/src/box/sql.c
> > +++ b/src/box/sql.c
> > @@ -765,10 +765,14 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
> >  			}
> >  		}
> >  		next_fieldno = fieldno + 1;
> > -		rc = sqlVdbeCompareMsgpack(&p, unpacked, i);
> > +		struct key_part *part = &unpacked->key_def->parts[i];
> > +		struct Mem *mem = unpacked->aMem + i;
> > +		struct coll *coll = part->coll;
> > +		/* In case of fail make rc equal to 0. */
> 
> The comment isn't really necessary I would say. Does not
> help much. The same in the other place below.
> 
Dropped.

> > +		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
> > +			rc = 0;
> >  		if (rc != 0) {
> > -			if (unpacked->key_def->parts[i].sort_order !=
> > -			    SORT_ORDER_ASC)
> > +			if (part->sort_order == SORT_ORDER_ASC)
> 
> Why did you change != to ==?
> 
I did this because function sqlVdbeCompareMsgpack() compared packed value as
left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed,
now MEM is left operand and packed value is right operand.

> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index da27cd191..8e176e418 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -2721,13 +2586,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
> >  	n = MIN(n, key2->nField);
> >  
> >  	for (i = 0; i != n; i++) {
> > -		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
> > +		struct key_part *part = &key2->key_def->parts[i];
> > +		struct Mem *mem = key2->aMem + i;
> > +		struct coll *coll = part->coll;
> > +		/* In case of fail make rc equal to 0. */
> > +		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
> > +			rc = 0;



Diff:


diff --git a/src/box/sql.c b/src/box/sql.c
index c1271ee0a..7471c3832 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -768,7 +768,6 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor,
 		struct key_part *part = &unpacked->key_def->parts[i];
 		struct Mem *mem = unpacked->aMem + i;
 		struct coll *coll = part->coll;
-		/* In case of fail make rc equal to 0. */
 		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
 			rc = 0;
 		if (rc != 0) {
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 8e176e418..62993fa4f 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2589,7 +2589,6 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
 		struct key_part *part = &key2->key_def->parts[i];
 		struct Mem *mem = key2->aMem + i;
 		struct coll *coll = part->coll;
-		/* In case of fail make rc equal to 0. */
 		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
 			rc = 0;
 		if (rc != 0) {

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-14  6:51             ` Mergen Imeev via Tarantool-patches
@ 2021-07-14 21:53               ` Vladislav Shpilevoy via Tarantool-patches
  2021-07-15  6:58                 ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-14 21:53 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

>>> +		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
>>> +			rc = 0;
>>>  		if (rc != 0) {
>>> -			if (unpacked->key_def->parts[i].sort_order !=
>>> -			    SORT_ORDER_ASC)
>>> +			if (part->sort_order == SORT_ORDER_ASC)
>>
>> Why did you change != to ==?
>>
> I did this because function sqlVdbeCompareMsgpack() compared packed value as
> left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed,
> now MEM is left operand and packed value is right operand.

Shouldn't you then change the other place too?

> @@ -2721,13 +2586,15 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
>  	n = MIN(n, key2->nField);
>  
>  	for (i = 0; i != n; i++) {
> -		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
> +		struct key_part *part = &key2->key_def->parts[i];
> +		struct Mem *mem = key2->aMem + i;
> +		struct coll *coll = part->coll;
> +		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
> +			rc = 0;
>  		if (rc != 0) {
> -			if (key2->key_def->parts[i].sort_order !=
> -			    SORT_ORDER_ASC) {
> -				rc = -rc;
> -			}
> -			return rc;
> +			if (part->sort_order != SORT_ORDER_ASC)
> +				return rc;
> +			return -rc;
>  		}

Here it was cmp(key1, key), now it is cmp(key2, key1). Shouldn't
then the sort_order check become `== SORT_ORDER_ASC`?

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

* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
  2021-07-14 21:53               ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-07-15  6:58                 ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-15  6:58 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answer below.

On 15.07.2021 00:53, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
>>>> +		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
>>>> +			rc = 0;
>>>>   		if (rc != 0) {
>>>> -			if (unpacked->key_def->parts[i].sort_order !=
>>>> -			    SORT_ORDER_ASC)
>>>> +			if (part->sort_order == SORT_ORDER_ASC)
>>> Why did you change != to ==?
>>>
>> I did this because function sqlVdbeCompareMsgpack() compared packed value as
>> left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed,
>> now MEM is left operand and packed value is right operand.
> Shouldn't you then change the other place too?
There I changed returned value instead of operation.
>
>> @@ -2721,13 +2586,15 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
>>   	n = MIN(n, key2->nField);
>>   
>>   	for (i = 0; i != n; i++) {
>> -		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
>> +		struct key_part *part = &key2->key_def->parts[i];
>> +		struct Mem *mem = key2->aMem + i;
>> +		struct coll *coll = part->coll;
>> +		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
>> +			rc = 0;
>>   		if (rc != 0) {
>> -			if (key2->key_def->parts[i].sort_order !=
>> -			    SORT_ORDER_ASC) {
>> -				rc = -rc;
>> -			}
>> -			return rc;
>> +			if (part->sort_order != SORT_ORDER_ASC)
>> +				return rc;
>> +			return -rc;
>>   		}
> Here it was cmp(key1, key), now it is cmp(key2, key1). Shouldn't
> then the sort_order check become `== SORT_ORDER_ASC`?
Although it is true that I have not changed '!=' to '==', now in case 
'!=' the
function returns rc, and in the case of '==' it returns -rc. Previously 
it was
-rc for '!=' and rc for '=='. Is there any other reason to change '!=' 
to '=='?

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

* Re: [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction
  2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
                   ` (3 preceding siblings ...)
  2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
@ 2021-07-15 20:44 ` Vladislav Shpilevoy via Tarantool-patches
  4 siblings, 0 replies; 20+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-15 20:44 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the good work!

LGTM.

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

* Re: [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values
  2021-07-16  8:57 ` [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values Mergen Imeev via Tarantool-patches
@ 2021-07-19  9:16   ` Timur Safin via Tarantool-patches
  0 siblings, 0 replies; 20+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-07-19  9:16 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

LGTM, and one minor comment we need to address eventually. 

: From: imeevma@tarantool.org <imeevma@tarantool.org>
: Subject: [PATCH v2 2/4] sql: allow to bind uuid values
: 
: After this patch, uuid values can be bound like any other supported by
: SQL values.
: 
: Part of #6164
: ---

: diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
: index ef8dcd693..115c52f96 100644
: --- a/src/box/sql/sqlInt.h
: +++ b/src/box/sql/sqlInt.h
: @@ -326,6 +326,8 @@ struct sql_vfs {
:  #define SQL_LIMIT_LIKE_PATTERN_LENGTH       8
:  #define SQL_LIMIT_TRIGGER_DEPTH             9
: 
: +struct tt_uuid;
: +
:  enum sql_ret_code {
:  	/** sql_step() has another row ready. */
:  	SQL_ROW = 1,
: @@ -634,6 +636,9 @@ int
:  sql_bind_zeroblob64(sql_stmt *, int,
:  			sql_uint64);
: 
: +int
: +sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid);
: +

As interface header I'd expect that every function put to sqlInt.h would be
properly documented, and wanted to complain that `sql_bind_uuid()` is not
accompanied with doxygen block ... but then discovered that majority of 
`sql_bind_*` functions (with exception of `sql_bind_boolean()`) were not 
documented at all, and actually this terse introduction looks more consistent
than if it would be documented :(

So, it looks like it's ok for today, but eventually we should return here and
make sure it's reasonably documented/commented out. 

:  /**
:   * Return the number of wildcards that should be bound to.
:   */
: diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
: index aaae12e41..8031ee0dc 100644
: --- a/src/box/sql/vdbeapi.c
: +++ b/src/box/sql/vdbeapi.c
: @@ -840,6 +840,16 @@ sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64
: n)
:  	return sql_bind_zeroblob(pStmt, i, n);
:  }
: 
: +int
: +sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
: +{
: +	struct Vdbe *p = (struct Vdbe *)stmt;
: +	if (vdbeUnbind(p, i) != 0 || sql_bind_type(p, i, "uuid") != 0)
: +		return -1;
: +	mem_set_uuid(&p->aVar[i - 1], uuid);
: +	return 0;
: +}
: +
:  int
:  sql_bind_parameter_count(const struct sql_stmt *stmt)
:  {
: diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-
: tap/gh-6164-uuid-follow-ups.test.lua
: index a8f662f77..4fc5052d8 100755
: --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
: +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
: @@ -1,6 +1,6 @@
:  #!/usr/bin/env tarantool
:  local test = require("sqltester")
: -test:plan(1)
: +test:plan(4)
: 
:  -- Make sure that function quote() can work with uuid.
:  test:do_execsql_test(
: @@ -11,4 +11,28 @@ test:do_execsql_test(
:          '11111111-1111-1111-1111-111111111111'
:      })
: 
: +-- Make sure that uuid value can be binded.
: +local uuid1 = require('uuid').fromstr('11111111-1111-1111-1111-
: 111111111111')
: +local uuid2 = require('uuid').fromstr('11111111-2222-1111-1111-
: 111111111111')
: +local uuid3 = require('uuid').fromstr('11111111-1111-3333-1111-
: 111111111111')
: +test:do_test(
: +    "gh-6164-2",
: +    function()
: +        return box.execute([[SELECT ?;]], {uuid1}).rows[1][1]
: +    end,
: +    uuid1)
: +test:do_test(
: +    "gh-6164-3",
: +    function()
: +        return box.execute([[SELECT $2;]], {123, uuid2}).rows[1][1]
: +    end,
: +    uuid2)
: +
: +test:do_test(
: +    "gh-6164-4",
: +    function()
: +        return box.execute([[SELECT :two;]], {{[":two"] =
: uuid3}}).rows[1][1]
: +    end,
: +    uuid3)
: +
:  test:finish_test()
: --
: 2.25.1

Regards,Timur


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

* [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values
  2021-07-16  8:57 Mergen Imeev via Tarantool-patches
@ 2021-07-16  8:57 ` Mergen Imeev via Tarantool-patches
  2021-07-19  9:16   ` Timur Safin via Tarantool-patches
  0 siblings, 1 reply; 20+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-16  8:57 UTC (permalink / raw)
  To: tsafin; +Cc: tarantool-patches

After this patch, uuid values can be bound like any other supported by
SQL values.

Part of #6164
---
 src/box/bind.c                                |  3 +++
 src/box/bind.h                                |  5 ++++
 src/box/lua/execute.c                         |  5 ++++
 src/box/sql/sqlInt.h                          |  5 ++++
 src/box/sql/vdbeapi.c                         | 10 +++++++
 test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 26 ++++++++++++++++++-
 6 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/src/box/bind.c b/src/box/bind.c
index d45a0f9a7..734f65186 100644
--- a/src/box/bind.c
+++ b/src/box/bind.c
@@ -191,6 +191,9 @@ sql_bind_column(struct sql_stmt *stmt, const struct sql_bind *p,
 	case MP_BIN:
 		return sql_bind_blob64(stmt, pos, (const void *) p->s, p->bytes,
 				       SQL_STATIC);
+	case MP_EXT:
+		assert(p->ext_type == MP_UUID);
+		return sql_bind_uuid(stmt, pos, &p->uuid);
 	default:
 		unreachable();
 	}
diff --git a/src/box/bind.h b/src/box/bind.h
index 568c558f3..143f010ce 100644
--- a/src/box/bind.h
+++ b/src/box/bind.h
@@ -40,6 +40,8 @@ extern "C" {
 #include <stdlib.h>
 
 #include "msgpuck.h"
+#include "uuid/tt_uuid.h"
+#include "mp_extension_types.h"
 
 struct sql_stmt;
 
@@ -59,6 +61,8 @@ struct sql_bind {
 	uint32_t bytes;
 	/** MessagePack type of the value. */
 	enum mp_type type;
+	/** Subtype of MP_EXT type. */
+	enum mp_extension_type ext_type;
 	/** Bind value. */
 	union {
 		bool b;
@@ -67,6 +71,7 @@ struct sql_bind {
 		uint64_t u64;
 		/** For string or blob. */
 		const char *s;
+		struct tt_uuid uuid;
 	};
 };
 
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 1b59b2e4a..62ccaf3f1 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -371,6 +371,10 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
 		bind->s = mp_decode_bin(&field.sval.data, &bind->bytes);
 		break;
 	case MP_EXT:
+		if (field.ext_type == MP_UUID) {
+			bind->uuid = *field.uuidval;
+			break;
+		}
 		diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA",
 			 sql_bind_name(bind));
 		return -1;
@@ -386,6 +390,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
 		unreachable();
 	}
 	bind->type = field.type;
+	bind->ext_type = field.ext_type;
 	lua_pop(L, lua_gettop(L) - idx);
 	return 0;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index ef8dcd693..115c52f96 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -326,6 +326,8 @@ struct sql_vfs {
 #define SQL_LIMIT_LIKE_PATTERN_LENGTH       8
 #define SQL_LIMIT_TRIGGER_DEPTH             9
 
+struct tt_uuid;
+
 enum sql_ret_code {
 	/** sql_step() has another row ready. */
 	SQL_ROW = 1,
@@ -634,6 +636,9 @@ int
 sql_bind_zeroblob64(sql_stmt *, int,
 			sql_uint64);
 
+int
+sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid);
+
 /**
  * Return the number of wildcards that should be bound to.
  */
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index aaae12e41..8031ee0dc 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -840,6 +840,16 @@ sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
 	return sql_bind_zeroblob(pStmt, i, n);
 }
 
+int
+sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
+{
+	struct Vdbe *p = (struct Vdbe *)stmt;
+	if (vdbeUnbind(p, i) != 0 || sql_bind_type(p, i, "uuid") != 0)
+		return -1;
+	mem_set_uuid(&p->aVar[i - 1], uuid);
+	return 0;
+}
+
 int
 sql_bind_parameter_count(const struct sql_stmt *stmt)
 {
diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
index a8f662f77..4fc5052d8 100755
--- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
+++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(1)
+test:plan(4)
 
 -- Make sure that function quote() can work with uuid.
 test:do_execsql_test(
@@ -11,4 +11,28 @@ test:do_execsql_test(
         '11111111-1111-1111-1111-111111111111'
     })
 
+-- Make sure that uuid value can be binded.
+local uuid1 = require('uuid').fromstr('11111111-1111-1111-1111-111111111111')
+local uuid2 = require('uuid').fromstr('11111111-2222-1111-1111-111111111111')
+local uuid3 = require('uuid').fromstr('11111111-1111-3333-1111-111111111111')
+test:do_test(
+    "gh-6164-2",
+    function()
+        return box.execute([[SELECT ?;]], {uuid1}).rows[1][1]
+    end,
+    uuid1)
+test:do_test(
+    "gh-6164-3",
+    function()
+        return box.execute([[SELECT $2;]], {123, uuid2}).rows[1][1]
+    end,
+    uuid2)
+
+test:do_test(
+    "gh-6164-4",
+    function()
+        return box.execute([[SELECT :two;]], {{[":two"] = uuid3}}).rows[1][1]
+    end,
+    uuid3)
+
 test:finish_test()
-- 
2.25.1


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

end of thread, other threads:[~2021-07-19  9:16 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() Mergen Imeev via Tarantool-patches
2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values Mergen Imeev via Tarantool-patches
2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches
2021-07-11 15:03   ` Vladislav Shpilevoy via Tarantool-patches
2021-07-11 17:51     ` Mergen Imeev via Tarantool-patches
2021-07-12 21:06       ` Vladislav Shpilevoy via Tarantool-patches
2021-07-13  8:04         ` Mergen Imeev via Tarantool-patches
2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
2021-07-11 15:05   ` Vladislav Shpilevoy via Tarantool-patches
2021-07-11 17:59     ` Mergen Imeev via Tarantool-patches
2021-07-12 21:09       ` Vladislav Shpilevoy via Tarantool-patches
2021-07-13  8:10         ` Mergen Imeev via Tarantool-patches
2021-07-13 20:39           ` Vladislav Shpilevoy via Tarantool-patches
2021-07-14  6:51             ` Mergen Imeev via Tarantool-patches
2021-07-14 21:53               ` Vladislav Shpilevoy via Tarantool-patches
2021-07-15  6:58                 ` Mergen Imeev via Tarantool-patches
2021-07-15 20:44 ` [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Vladislav Shpilevoy via Tarantool-patches
2021-07-16  8:57 Mergen Imeev via Tarantool-patches
2021-07-16  8:57 ` [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values Mergen Imeev via Tarantool-patches
2021-07-19  9:16   ` Timur Safin via Tarantool-patches

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