* [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote()
2021-07-16 8:57 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction 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
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values Mergen Imeev via Tarantool-patches
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-16 8:57 UTC (permalink / raw)
To: tsafin; +Cc: tarantool-patches
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
---
src/box/sql/func.c | 24 ++++++++++++-------
test/sql-tap/engine.cfg | 3 +++
test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 14 +++++++++++
3 files changed, 32 insertions(+), 9 deletions(-)
create mode 100755 test/sql-tap/gh-6164-uuid-follow-ups.test.lua
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 84768f17c..d2edda6d3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1095,8 +1095,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]);
@@ -1110,14 +1110,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]);
@@ -1143,7 +1149,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]);
@@ -1171,7 +1177,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()
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote()
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() Mergen Imeev via Tarantool-patches
@ 2021-07-19 9:16 ` Timur Safin via Tarantool-patches
0 siblings, 0 replies; 19+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-07-19 9:16 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
LGTM
: From: imeevma@tarantool.org <imeevma@tarantool.org>
: Subject: [PATCH v2 1/4] 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
: ---
: src/box/sql/func.c | 24 ++++++++++++-------
: test/sql-tap/engine.cfg | 3 +++
: test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 14 +++++++++++
: 3 files changed, 32 insertions(+), 9 deletions(-)
: create mode 100755 test/sql-tap/gh-6164-uuid-follow-ups.test.lua
:
: diff --git a/src/box/sql/func.c b/src/box/sql/func.c
: index 84768f17c..d2edda6d3 100644
: --- a/src/box/sql/func.c
: +++ b/src/box/sql/func.c
: @@ -1095,8 +1095,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]);
: @@ -1110,14 +1110,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]);
: @@ -1143,7 +1149,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]);
: @@ -1171,7 +1177,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()
: --
: 2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Tarantool-patches] [PATCH v2 2/4] sql: allow to bind uuid values
2021-07-16 8:57 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() 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
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
3 siblings, 1 reply; 19+ 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] 19+ 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; 19+ 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] 19+ messages in thread
* [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
2021-07-16 8:57 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 1/4] sql: introduce uuid to quote() 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-16 8:57 ` Mergen Imeev via Tarantool-patches
2021-07-19 9:17 ` Timur Safin via Tarantool-patches
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
3 siblings, 1 reply; 19+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-16 8:57 UTC (permalink / raw)
To: tsafin; +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 | 30 +++-
src/box/sql/mem.c | 144 +++++++++---------
src/box/sql/mem.h | 10 +-
src/box/sql/vdbe.c | 8 +-
src/box/sql/where.c | 22 ++-
test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 45 +++++-
6 files changed, 168 insertions(+), 91 deletions(-)
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index d2edda6d3..6ca852dec 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]);
}
@@ -1054,9 +1059,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]);
}
/**
@@ -1824,7 +1835,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 cdb55f858..a8cde6769 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -60,6 +60,44 @@ enum {
STR_VALUE_MAX_LEN = 128,
};
+/**
+ * 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)
{
@@ -2030,6 +2068,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
@@ -2463,82 +2531,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 97aba9ab4..dd8666f53 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -698,6 +698,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
@@ -963,8 +971,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 cc698b715..9e763ed85 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1825,7 +1825,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()
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar()
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches
@ 2021-07-19 9:17 ` Timur Safin via Tarantool-patches
0 siblings, 0 replies; 19+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-07-19 9:17 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
LGTM, yup, it's much simpler now!
: From: imeevma@tarantool.org <imeevma@tarantool.org>
: Subject: [PATCH v2 3/4] 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
: @@ -2030,6 +2068,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
: @@ -2463,82 +2531,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)
: {
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
2021-07-16 8:57 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches
` (2 preceding siblings ...)
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() 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
3 siblings, 1 reply; 19+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-16 8:57 UTC (permalink / raw)
To: tsafin; +Cc: tarantool-patches
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
---
src/box/sql.c | 9 +-
src/box/sql/mem.c | 289 +++++-------------
src/box/sql/mem.h | 24 +-
test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 14 +-
4 files changed, 107 insertions(+), 229 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index 790ca7f70..7471c3832 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -765,10 +765,13 @@ 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;
+ 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 a8cde6769..fa80f6d5a 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2098,35 +2098,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 *
@@ -2557,183 +2599,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)
@@ -2744,13 +2609,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;
}
}
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index dd8666f53..645d0ee27 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -706,6 +706,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
@@ -983,20 +993,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] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
2021-07-16 8:57 ` [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack() Mergen Imeev via Tarantool-patches
@ 2021-07-19 9:16 ` Timur Safin via Tarantool-patches
2021-07-19 10:07 ` Mergen Imeev via Tarantool-patches
0 siblings, 1 reply; 19+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-07-19 9:16 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
LGTM, but with 1 question...
: From: imeevma@tarantool.org <imeevma@tarantool.org>
: Subject: [PATCH v2 4/4] 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
: ---
: src/box/sql.c | 9 +-
: src/box/sql/mem.c | 289 +++++-------------
: src/box/sql/mem.h | 24 +-
: test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 14 +-
: 4 files changed, 107 insertions(+), 229 deletions(-)
:
: diff --git a/src/box/sql.c b/src/box/sql.c
: index 790ca7f70..7471c3832 100644
: --- a/src/box/sql.c
: +++ b/src/box/sql.c
: @@ -765,10 +765,13 @@ 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;
: + 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 a8cde6769..fa80f6d5a 100644
: --- a/src/box/sql/mem.c
: +++ b/src/box/sql/mem.c
: @@ -2098,35 +2098,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);
Decimals are coming here? Yes?
: + 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 *
Regards,
Timur
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()
2021-07-19 9:16 ` Timur Safin via Tarantool-patches
@ 2021-07-19 10:07 ` Mergen Imeev via Tarantool-patches
0 siblings, 0 replies; 19+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-19 10:07 UTC (permalink / raw)
To: Timur Safin; +Cc: tarantool-patches
Hi! Thank you for the review. My answer below.
On 19.07.2021 12:16, Timur Safin wrote:
> LGTM, but with 1 question...
>
> : From: imeevma@tarantool.org <imeevma@tarantool.org>
> : Subject: [PATCH v2 4/4] 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
> : ---
> : src/box/sql.c | 9 +-
> : src/box/sql/mem.c | 289 +++++-------------
> : src/box/sql/mem.h | 24 +-
> : test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 14 +-
> : 4 files changed, 107 insertions(+), 229 deletions(-)
> :
> : diff --git a/src/box/sql.c b/src/box/sql.c
> : index 790ca7f70..7471c3832 100644
> : --- a/src/box/sql.c
> : +++ b/src/box/sql.c
> : @@ -765,10 +765,13 @@ 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;
> : + 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 a8cde6769..fa80f6d5a 100644
> : --- a/src/box/sql/mem.c
> : +++ b/src/box/sql/mem.c
> : @@ -2098,35 +2098,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);
>
> Decimals are coming here? Yes?
Yes. If you want I can give you branch where DECIMAL is partially done.
> : + 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 *
>
> Regards,
> Timur
>
^ permalink raw reply [flat|nested] 19+ 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
@ 2021-07-10 14:33 ` Mergen Imeev via Tarantool-patches
2021-07-11 15:05 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ messages in thread