* [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction @ 2021-07-16 8:57 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 ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-07-16 8:57 UTC (permalink / raw) To: tsafin; +Cc: tarantool-patches After addition of UUID to SQL a few new problems appeared. This patch fixes such problems. https://github.com/tarantool/tarantool/issues/6164 https://github.com/tarantool/tarantool/tree/imeevma/gh-6164-uuid-follow-ups Changes in v2: - Fixed problem with wrong comparison in MIN()/MAX() functions. - Fixed problem with wrong comparison in ORDER BY. Mergen Imeev (4): sql: introduce uuid to quote() sql: allow to bind uuid values sql: introduce mem_cmp_scalar() sql: introduce mem_cmp_msgpack() src/box/bind.c | 3 + src/box/bind.h | 5 + src/box/lua/execute.c | 5 + src/box/sql.c | 9 +- src/box/sql/func.c | 54 ++- src/box/sql/mem.c | 427 ++++++------------ src/box/sql/mem.h | 34 +- src/box/sql/sqlInt.h | 5 + src/box/sql/vdbe.c | 8 +- src/box/sql/vdbeapi.c | 10 + src/box/sql/where.c | 22 +- test/sql-tap/engine.cfg | 3 + test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 93 ++++ 13 files changed, 354 insertions(+), 324 deletions(-) create mode 100755 test/sql-tap/gh-6164-uuid-follow-ups.test.lua -- 2.25.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ messages in thread
* [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction @ 2021-07-10 14:33 Mergen Imeev via Tarantool-patches 2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches 0 siblings, 1 reply; 15+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches After addition of UUID to SQL a few new problems appeared. This patch fixes such problems. https://github.com/tarantool/tarantool/issues/6164 https://github.com/tarantool/tarantool/tree/imeevma/gh-6164-uuid-follow-ups Changes in v2: - Fixed problem with wrong comparison in MIN()/MAX() functions. - Fixed problem with wrong comparison in ORDER BY. Mergen Imeev (4): sql: introduce uuid to quote() sql: allow to bind uuid values sql: introduce mem_cmp_scalar() sql: introduce mem_cmp_msgpack() src/box/bind.c | 3 + src/box/bind.h | 5 + src/box/lua/execute.c | 5 + src/box/sql.c | 9 +- src/box/sql/func.c | 38 +- src/box/sql/mem.c | 416 ++++++------------ src/box/sql/mem.h | 33 +- src/box/sql/sqlInt.h | 5 + src/box/sql/vdbe.c | 2 +- src/box/sql/vdbeapi.c | 10 + src/box/sql/where.c | 6 +- test/sql-tap/engine.cfg | 3 + test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 93 ++++ 13 files changed, 307 insertions(+), 321 deletions(-) create mode 100755 test/sql-tap/gh-6164-uuid-follow-ups.test.lua -- 2.25.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() 2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 ` Mergen Imeev via Tarantool-patches 2021-07-11 15:03 ` Vladislav Shpilevoy via Tarantool-patches 0 siblings, 1 reply; 15+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-07-10 14:33 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches This patch introduces the mem_cmp_scalar() function that compares two MEMs using SCALAR rules. MEMs must be scalars. Prior to this patch, there was a function that used SCALAR rules to compare two MEMs, but its design became overly complex as new types appeared. Part of #6164 --- src/box/sql/func.c | 14 +- src/box/sql/mem.c | 140 ++++++++---------- src/box/sql/mem.h | 10 +- src/box/sql/vdbe.c | 2 +- src/box/sql/where.c | 6 +- test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 45 +++++- 6 files changed, 128 insertions(+), 89 deletions(-) diff --git a/src/box/sql/func.c b/src/box/sql/func.c index aa565277c..dc99bd390 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) for (i = 1; i < argc; i++) { if (mem_is_null(argv[i])) return; - if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= - 0) { - testcase(mask == 0); + int res; + mem_cmp_scalar(argv[iBest], argv[i], &res, pColl); + if ((res ^ mask) >= 0) iBest = i; - } } sql_result_value(context, argv[iBest]); } @@ -1057,9 +1056,10 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv) { struct coll *pColl = sqlGetFuncCollSeq(context); UNUSED_PARAMETER(NotUsed); - if (sqlMemCompare(argv[0], argv[1], pColl) != 0) { + int res; + mem_cmp_scalar(argv[0], argv[1], &res, pColl); + if (res != 0) sql_result_value(context, argv[0]); - } } /** @@ -1827,7 +1827,7 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) * comparison is inverted. */ bool is_max = (func->flags & SQL_FUNC_MAX) != 0; - cmp = sqlMemCompare(pBest, pArg, pColl); + mem_cmp_scalar(pBest, pArg, &cmp, pColl); if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { mem_copy(pBest, pArg); } else { diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 2595e2fd4..576596c9f 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -59,6 +59,40 @@ enum { BUF_SIZE = 32, }; +enum mem_class { + MEM_CLASS_NULL, + MEM_CLASS_BOOL, + MEM_CLASS_NUMBER, + MEM_CLASS_STR, + MEM_CLASS_BIN, + MEM_CLASS_UUID, + mem_class_max, +}; + +static inline enum mem_class +mem_type_class(enum mem_type type) +{ + switch (type) { + case MEM_TYPE_NULL: + return MEM_CLASS_NULL; + case MEM_TYPE_UINT: + case MEM_TYPE_INT: + case MEM_TYPE_DOUBLE: + return MEM_CLASS_NUMBER; + case MEM_TYPE_STR: + return MEM_CLASS_STR; + case MEM_TYPE_BIN: + return MEM_CLASS_BIN; + case MEM_TYPE_BOOL: + return MEM_CLASS_BOOL; + case MEM_TYPE_UUID: + return MEM_CLASS_UUID; + default: + break; + } + return mem_class_max; +} + bool mem_is_field_compatible(const struct Mem *mem, enum field_type type) { @@ -2009,6 +2043,36 @@ mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result) return 0; } +int +mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, + const struct coll *coll) +{ + enum mem_class class_a = mem_type_class(a->type); + enum mem_class class_b = mem_type_class(b->type); + if (class_a != class_b) { + *result = class_a - class_b; + return 0; + } + switch (class_a) { + case MEM_CLASS_NULL: + *result = 0; + return 0; + case MEM_CLASS_BOOL: + return mem_cmp_bool(a, b, result); + case MEM_CLASS_NUMBER: + return mem_cmp_num(a, b, result); + case MEM_CLASS_STR: + return mem_cmp_str(a, b, result, coll); + case MEM_CLASS_BIN: + return mem_cmp_bin(a, b, result); + case MEM_CLASS_UUID: + return mem_cmp_uuid(a, b, result); + default: + unreachable(); + } + return 0; +} + /* * Both *pMem1 and *pMem2 contain string values. Compare the two values * using the collation sequence pColl. As usual, return a negative , zero @@ -2440,82 +2504,6 @@ sqlVdbeMemTooBig(Mem * p) return 0; } -/* - * Compare the values contained by the two memory cells, returning - * negative, zero or positive if pMem1 is less than, equal to, or greater - * than pMem2. Sorting order is NULL's first, followed by numbers (integers - * and reals) sorted numerically, followed by text ordered by the collating - * sequence pColl and finally blob's ordered by memcmp(). - * - * Two NULL values are considered equal by this function. - */ -int -sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl) -{ - int res; - - enum mem_type type1 = pMem1->type; - enum mem_type type2 = pMem2->type; - - /* If one value is NULL, it is less than the other. If both values - * are NULL, return 0. - */ - if (((type1 | type2) & MEM_TYPE_NULL) != 0) - return (int)(type2 == MEM_TYPE_NULL) - - (int)(type1 == MEM_TYPE_NULL); - - if (((type1 | type2) & MEM_TYPE_BOOL) != 0) { - if ((type1 & type2 & MEM_TYPE_BOOL) != 0) { - if (pMem1->u.b == pMem2->u.b) - return 0; - if (pMem1->u.b) - return 1; - return -1; - } - if (type2 == MEM_TYPE_BOOL) - return +1; - return -1; - } - - if (((type1 | type2) & MEM_TYPE_UUID) != 0) { - if (mem_cmp_uuid(pMem1, pMem2, &res) == 0) - return res; - if (type1 != MEM_TYPE_UUID) - return +1; - return -1; - } - - /* At least one of the two values is a number - */ - if (((type1 | type2) & - (MEM_TYPE_INT | MEM_TYPE_UINT | MEM_TYPE_DOUBLE)) != 0) { - if (!mem_is_num(pMem1)) - return +1; - if (!mem_is_num(pMem2)) - return -1; - mem_cmp_num(pMem1, pMem2, &res); - return res; - } - - /* If one value is a string and the other is a blob, the string is less. - * If both are strings, compare using the collating functions. - */ - if (((type1 | type2) & MEM_TYPE_STR) != 0) { - if (type1 != MEM_TYPE_STR) { - return 1; - } - if (type2 != MEM_TYPE_STR) { - return -1; - } - mem_cmp_str(pMem1, pMem2, &res, pColl); - return res; - } - - /* Both values must be blobs. Compare using memcmp(). */ - mem_cmp_bin(pMem1, pMem2, &res); - return res; -} - int sql_vdbemem_finalize(struct Mem *mem, struct func *func) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index b3cd5c545..bbb99c4d2 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -696,6 +696,14 @@ mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result); int mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result); +/** + * Compare two MEMs using SCALAR rules and return the result of comparison. MEMs + * should be scalars. Original MEMs are not changed. + */ +int +mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, + const struct coll *coll); + /** * Convert the given MEM to INTEGER. This function and the function below define * the rules that are used to convert values of all other types to INTEGER. In @@ -961,8 +969,6 @@ int sqlVdbeMemTooBig(Mem *); #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\ ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0) -int sqlMemCompare(const Mem *, const Mem *, const struct coll *); - /** MEM manipulate functions. */ /** diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 32d02d96e..220e8b269 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1828,7 +1828,7 @@ case OP_Compare: { assert(i < (int)def->part_count); struct coll *coll = def->parts[i].coll; bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC; - iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll); + mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll); if (iCompare) { if (is_rev) iCompare = -iCompare; diff --git a/src/box/sql/where.c b/src/box/sql/where.c index e5f35fbf8..dadc6d4a2 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ rc = sql_stat4_column(db, samples[i].sample_key, nEq, &pVal); if (rc == 0 && p1 != NULL) { - int res = sqlMemCompare(p1, pVal, coll); + int res; + mem_cmp_scalar(p1, pVal, &res, coll); if (res >= 0) nLower++; } if (rc == 0 && p2 != NULL) { - int res = sqlMemCompare(p2, pVal, coll); + int res; + mem_cmp_scalar(p2, pVal, &res, coll); if (res >= 0) nUpper++; } diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua index 4fc5052d8..6b4a811c3 100755 --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(4) +test:plan(8) -- Make sure that function quote() can work with uuid. test:do_execsql_test( @@ -35,4 +35,47 @@ test:do_test( end, uuid3) +-- +-- Make sure a comparison that includes a UUID and follows the SCALAR rules is +-- working correctly. +-- +box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, s SCALAR);]]) +box.execute([[INSERT INTO t VALUES (1, ?)]], {uuid1}) + +test:do_execsql_test( + "gh-6164-5", + [[ + SELECT GREATEST(i, s, x'33', 'something') FROM t; + ]], { + uuid1 + }) + +test:do_execsql_test( + "gh-6164-6", + [[ + SELECT LEAST(i, s, x'33', 'something') FROM t; + ]], { + 1 + }) + +box.execute([[INSERT INTO t VALUES (2, 2);]]) + +test:do_execsql_test( + "gh-6164-7", + [[ + SELECT MAX(s) FROM t; + ]], { + uuid1 + }) + +test:do_execsql_test( + "gh-6164-8", + [[ + SELECT MIN(s) FROM t; + ]], { + 2 + }) + +box.execute([[DROP TABLE t;]]) + test:finish_test() -- 2.25.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() 2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches @ 2021-07-11 15:03 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-11 17:51 ` Mergen Imeev via Tarantool-patches 0 siblings, 1 reply; 15+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-11 15:03 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches Hi! Thanks for the patch! See 3 comments below. > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index aa565277c..dc99bd390 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) > for (i = 1; i < argc; i++) { > if (mem_is_null(argv[i])) > return; > - if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= > - 0) { > - testcase(mask == 0); > + int res; > + mem_cmp_scalar(argv[iBest], argv[i], &res, pColl); > + if ((res ^ mask) >= 0) 1. It seems that under certain conditions if cmp_scalar fails, res remains not initialized. Which can lead to behaviour changing from run to run. The same in the other places below. > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index 2595e2fd4..576596c9f 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -59,6 +59,40 @@ enum { > BUF_SIZE = 32, > }; > > +enum mem_class { > + MEM_CLASS_NULL, > + MEM_CLASS_BOOL, > + MEM_CLASS_NUMBER, > + MEM_CLASS_STR, > + MEM_CLASS_BIN, > + MEM_CLASS_UUID, > + mem_class_max, > +}; 2. It might make sense to add a comment that these must be sorted exactly like enum mp_class. > + > +static inline enum mem_class > +mem_type_class(enum mem_type type) > +{ > + switch (type) { > + case MEM_TYPE_NULL: > + return MEM_CLASS_NULL; > + case MEM_TYPE_UINT: > + case MEM_TYPE_INT: > + case MEM_TYPE_DOUBLE: > + return MEM_CLASS_NUMBER; > + case MEM_TYPE_STR: > + return MEM_CLASS_STR; > + case MEM_TYPE_BIN: > + return MEM_CLASS_BIN; > + case MEM_TYPE_BOOL: > + return MEM_CLASS_BOOL; > + case MEM_TYPE_UUID: > + return MEM_CLASS_UUID; 3. It might work faster without branching if done like 'static enum mp_class mp_classes[]' - would allow to take the class for any type as simple as an array access operation. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() 2021-07-11 15:03 ` Vladislav Shpilevoy via Tarantool-patches @ 2021-07-11 17:51 ` Mergen Imeev via Tarantool-patches 2021-07-12 21:06 ` Vladislav Shpilevoy via Tarantool-patches 0 siblings, 1 reply; 15+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-07-11 17:51 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thank you for the review! My answers, diff and new patch below. On Sun, Jul 11, 2021 at 05:03:34PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 3 comments below. > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index aa565277c..dc99bd390 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) > > for (i = 1; i < argc; i++) { > > if (mem_is_null(argv[i])) > > return; > > - if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= > > - 0) { > > - testcase(mask == 0); > > + int res; > > + mem_cmp_scalar(argv[iBest], argv[i], &res, pColl); > > + if ((res ^ mask) >= 0) > > 1. It seems that under certain conditions if cmp_scalar fails, res remains > not initialized. Which can lead to behaviour changing from run to run. The > same in the other places below. > Fixed, added checks. Also, I believe there shoudn't be any problem when we compare two values of the same type. Right now all comparison functions can return -1 when they were given wrong argument, but I believe I will fix this when I remove implicit cast. I want them to accept only values of predefined types and return result of comparison. Then we can get rid of unnecessary checks. > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index 2595e2fd4..576596c9f 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -59,6 +59,40 @@ enum { > > BUF_SIZE = 32, > > }; > > > > +enum mem_class { > > + MEM_CLASS_NULL, > > + MEM_CLASS_BOOL, > > + MEM_CLASS_NUMBER, > > + MEM_CLASS_STR, > > + MEM_CLASS_BIN, > > + MEM_CLASS_UUID, > > + mem_class_max, > > +}; > > 2. It might make sense to add a comment that these must be sorted > exactly like enum mp_class. > Added. > > + > > +static inline enum mem_class > > +mem_type_class(enum mem_type type) > > +{ > > + switch (type) { > > + case MEM_TYPE_NULL: > > + return MEM_CLASS_NULL; > > + case MEM_TYPE_UINT: > > + case MEM_TYPE_INT: > > + case MEM_TYPE_DOUBLE: > > + return MEM_CLASS_NUMBER; > > + case MEM_TYPE_STR: > > + return MEM_CLASS_STR; > > + case MEM_TYPE_BIN: > > + return MEM_CLASS_BIN; > > + case MEM_TYPE_BOOL: > > + return MEM_CLASS_BOOL; > > + case MEM_TYPE_UUID: > > + return MEM_CLASS_UUID; > > 3. It might work faster without branching if done like > 'static enum mp_class mp_classes[]' - would allow to take > the class for any type as simple as an array access > operation. This cannot be done directly in current design since value in enum mem_type are not sequential numbers. However, this can be done using something like this: diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 4062ff4b3..05a3fb699 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -73,28 +73,27 @@ enum mem_class { mem_class_max, }; +enum mem_class mem_classes[] = { + /** MEM_TYPE_NULL = */ MEM_CLASS_NULL, + /** MEM_TYPE_UINT = */ MEM_CLASS_NUMBER, + /** MEM_TYPE_INT = */ MEM_CLASS_NUMBER, + /** MEM_TYPE_STR = */ MEM_CLASS_STR, + /** MEM_TYPE_BIN = */ MEM_CLASS_BIN, + /** MEM_TYPE_ARRAY = */ mem_class_max, + /** MEM_TYPE_MAP = */ mem_class_max, + /** MEM_TYPE_BOOL = */ MEM_CLASS_BOOL, + /** MEM_TYPE_DOUBLE = */ MEM_CLASS_NUMBER, + /** MEM_TYPE_UUID = */ MEM_CLASS_UUID, + /** MEM_TYPE_INVALID = */ mem_class_max, + /** MEM_TYPE_FRAME = */ mem_class_max, + /** MEM_TYPE_PTR = */ mem_class_max, + /** MEM_TYPE_AGG = */ mem_class_max, +}; + static inline enum mem_class mem_type_class(enum mem_type type) { - switch (type) { - case MEM_TYPE_NULL: - return MEM_CLASS_NULL; - case MEM_TYPE_UINT: - case MEM_TYPE_INT: - case MEM_TYPE_DOUBLE: - return MEM_CLASS_NUMBER; - case MEM_TYPE_STR: - return MEM_CLASS_STR; - case MEM_TYPE_BIN: - return MEM_CLASS_BIN; - case MEM_TYPE_BOOL: - return MEM_CLASS_BOOL; - case MEM_TYPE_UUID: - return MEM_CLASS_UUID; - default: - break; - } - return mem_class_max; + return mem_classes[ffs(type) - 1]; } bool We can use macro instead of static inline function. What do you think? Also we can think of way to solve this problem using bit-wise operations, but it looks too complicated. Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index dc99bd390..efb14f23e 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -145,7 +145,13 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) if (mem_is_null(argv[i])) return; int res; - mem_cmp_scalar(argv[iBest], argv[i], &res, pColl); + if (mem_cmp_scalar(argv[iBest], argv[i], &res, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(argv[i]), + mem_type_to_str(argv[iBest])); + context->is_aborted = true; + return; + } if ((res ^ mask) >= 0) iBest = i; } @@ -1057,7 +1063,12 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv) struct coll *pColl = sqlGetFuncCollSeq(context); UNUSED_PARAMETER(NotUsed); int res; - mem_cmp_scalar(argv[0], argv[1], &res, pColl); + if (mem_cmp_scalar(argv[0], argv[1], &res, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(argv[1]), mem_type_to_str(argv[0])); + context->is_aborted = true; + return; + } if (res != 0) sql_result_value(context, argv[0]); } @@ -1827,7 +1838,12 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) * comparison is inverted. */ bool is_max = (func->flags & SQL_FUNC_MAX) != 0; - mem_cmp_scalar(pBest, pArg, &cmp, pColl); + if (mem_cmp_scalar(pBest, pArg, &cmp, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(pArg), mem_type_to_str(pBest)); + context->is_aborted = true; + return; + } if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { mem_copy(pBest, pArg); } else { diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 576596c9f..da27cd191 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -59,6 +59,10 @@ enum { BUF_SIZE = 32, }; +/** + * Analogue of enum mp_class for enum mp_type. The order of the classes must be + * the same as in the enum mp_class. + */ enum mem_class { MEM_CLASS_NULL, MEM_CLASS_BOOL, New patch: commit c65ccc80c55501035ac6d5ab71b0e30460aee0fd Author: Mergen Imeev <imeevma@gmail.com> Date: Sat Jul 10 13:56:39 2021 +0300 sql: introduce mem_cmp_scalar() This patch introduces the mem_cmp_scalar() function that compares two MEMs using SCALAR rules. MEMs must be scalars. Prior to this patch, there was a function that used SCALAR rules to compare two MEMs, but its design became overly complex as new types appeared. Part of #6164 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index aa565277c..efb14f23e 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -144,11 +144,16 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) for (i = 1; i < argc; i++) { if (mem_is_null(argv[i])) return; - if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= - 0) { - testcase(mask == 0); - iBest = i; + int res; + if (mem_cmp_scalar(argv[iBest], argv[i], &res, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(argv[i]), + mem_type_to_str(argv[iBest])); + context->is_aborted = true; + return; } + if ((res ^ mask) >= 0) + iBest = i; } sql_result_value(context, argv[iBest]); } @@ -1057,9 +1062,15 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv) { struct coll *pColl = sqlGetFuncCollSeq(context); UNUSED_PARAMETER(NotUsed); - if (sqlMemCompare(argv[0], argv[1], pColl) != 0) { - sql_result_value(context, argv[0]); + int res; + if (mem_cmp_scalar(argv[0], argv[1], &res, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(argv[1]), mem_type_to_str(argv[0])); + context->is_aborted = true; + return; } + if (res != 0) + sql_result_value(context, argv[0]); } /** @@ -1827,7 +1838,12 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) * comparison is inverted. */ bool is_max = (func->flags & SQL_FUNC_MAX) != 0; - cmp = sqlMemCompare(pBest, pArg, pColl); + if (mem_cmp_scalar(pBest, pArg, &cmp, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(pArg), mem_type_to_str(pBest)); + context->is_aborted = true; + return; + } if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { mem_copy(pBest, pArg); } else { diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 2595e2fd4..da27cd191 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -59,6 +59,44 @@ enum { BUF_SIZE = 32, }; +/** + * Analogue of enum mp_class for enum mp_type. The order of the classes must be + * the same as in the enum mp_class. + */ +enum mem_class { + MEM_CLASS_NULL, + MEM_CLASS_BOOL, + MEM_CLASS_NUMBER, + MEM_CLASS_STR, + MEM_CLASS_BIN, + MEM_CLASS_UUID, + mem_class_max, +}; + +static inline enum mem_class +mem_type_class(enum mem_type type) +{ + switch (type) { + case MEM_TYPE_NULL: + return MEM_CLASS_NULL; + case MEM_TYPE_UINT: + case MEM_TYPE_INT: + case MEM_TYPE_DOUBLE: + return MEM_CLASS_NUMBER; + case MEM_TYPE_STR: + return MEM_CLASS_STR; + case MEM_TYPE_BIN: + return MEM_CLASS_BIN; + case MEM_TYPE_BOOL: + return MEM_CLASS_BOOL; + case MEM_TYPE_UUID: + return MEM_CLASS_UUID; + default: + break; + } + return mem_class_max; +} + bool mem_is_field_compatible(const struct Mem *mem, enum field_type type) { @@ -2009,6 +2047,36 @@ mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result) return 0; } +int +mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, + const struct coll *coll) +{ + enum mem_class class_a = mem_type_class(a->type); + enum mem_class class_b = mem_type_class(b->type); + if (class_a != class_b) { + *result = class_a - class_b; + return 0; + } + switch (class_a) { + case MEM_CLASS_NULL: + *result = 0; + return 0; + case MEM_CLASS_BOOL: + return mem_cmp_bool(a, b, result); + case MEM_CLASS_NUMBER: + return mem_cmp_num(a, b, result); + case MEM_CLASS_STR: + return mem_cmp_str(a, b, result, coll); + case MEM_CLASS_BIN: + return mem_cmp_bin(a, b, result); + case MEM_CLASS_UUID: + return mem_cmp_uuid(a, b, result); + default: + unreachable(); + } + return 0; +} + /* * Both *pMem1 and *pMem2 contain string values. Compare the two values * using the collation sequence pColl. As usual, return a negative , zero @@ -2440,82 +2508,6 @@ sqlVdbeMemTooBig(Mem * p) return 0; } -/* - * Compare the values contained by the two memory cells, returning - * negative, zero or positive if pMem1 is less than, equal to, or greater - * than pMem2. Sorting order is NULL's first, followed by numbers (integers - * and reals) sorted numerically, followed by text ordered by the collating - * sequence pColl and finally blob's ordered by memcmp(). - * - * Two NULL values are considered equal by this function. - */ -int -sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl) -{ - int res; - - enum mem_type type1 = pMem1->type; - enum mem_type type2 = pMem2->type; - - /* If one value is NULL, it is less than the other. If both values - * are NULL, return 0. - */ - if (((type1 | type2) & MEM_TYPE_NULL) != 0) - return (int)(type2 == MEM_TYPE_NULL) - - (int)(type1 == MEM_TYPE_NULL); - - if (((type1 | type2) & MEM_TYPE_BOOL) != 0) { - if ((type1 & type2 & MEM_TYPE_BOOL) != 0) { - if (pMem1->u.b == pMem2->u.b) - return 0; - if (pMem1->u.b) - return 1; - return -1; - } - if (type2 == MEM_TYPE_BOOL) - return +1; - return -1; - } - - if (((type1 | type2) & MEM_TYPE_UUID) != 0) { - if (mem_cmp_uuid(pMem1, pMem2, &res) == 0) - return res; - if (type1 != MEM_TYPE_UUID) - return +1; - return -1; - } - - /* At least one of the two values is a number - */ - if (((type1 | type2) & - (MEM_TYPE_INT | MEM_TYPE_UINT | MEM_TYPE_DOUBLE)) != 0) { - if (!mem_is_num(pMem1)) - return +1; - if (!mem_is_num(pMem2)) - return -1; - mem_cmp_num(pMem1, pMem2, &res); - return res; - } - - /* If one value is a string and the other is a blob, the string is less. - * If both are strings, compare using the collating functions. - */ - if (((type1 | type2) & MEM_TYPE_STR) != 0) { - if (type1 != MEM_TYPE_STR) { - return 1; - } - if (type2 != MEM_TYPE_STR) { - return -1; - } - mem_cmp_str(pMem1, pMem2, &res, pColl); - return res; - } - - /* Both values must be blobs. Compare using memcmp(). */ - mem_cmp_bin(pMem1, pMem2, &res); - return res; -} - int sql_vdbemem_finalize(struct Mem *mem, struct func *func) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index b3cd5c545..bbb99c4d2 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -696,6 +696,14 @@ mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result); int mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result); +/** + * Compare two MEMs using SCALAR rules and return the result of comparison. MEMs + * should be scalars. Original MEMs are not changed. + */ +int +mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, + const struct coll *coll); + /** * Convert the given MEM to INTEGER. This function and the function below define * the rules that are used to convert values of all other types to INTEGER. In @@ -961,8 +969,6 @@ int sqlVdbeMemTooBig(Mem *); #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\ ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0) -int sqlMemCompare(const Mem *, const Mem *, const struct coll *); - /** MEM manipulate functions. */ /** diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 32d02d96e..220e8b269 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1828,7 +1828,7 @@ case OP_Compare: { assert(i < (int)def->part_count); struct coll *coll = def->parts[i].coll; bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC; - iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll); + mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll); if (iCompare) { if (is_rev) iCompare = -iCompare; diff --git a/src/box/sql/where.c b/src/box/sql/where.c index e5f35fbf8..dadc6d4a2 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ rc = sql_stat4_column(db, samples[i].sample_key, nEq, &pVal); if (rc == 0 && p1 != NULL) { - int res = sqlMemCompare(p1, pVal, coll); + int res; + mem_cmp_scalar(p1, pVal, &res, coll); if (res >= 0) nLower++; } if (rc == 0 && p2 != NULL) { - int res = sqlMemCompare(p2, pVal, coll); + int res; + mem_cmp_scalar(p2, pVal, &res, coll); if (res >= 0) nUpper++; } diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua index 4fc5052d8..6b4a811c3 100755 --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(4) +test:plan(8) -- Make sure that function quote() can work with uuid. test:do_execsql_test( @@ -35,4 +35,47 @@ test:do_test( end, uuid3) +-- +-- Make sure a comparison that includes a UUID and follows the SCALAR rules is +-- working correctly. +-- +box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, s SCALAR);]]) +box.execute([[INSERT INTO t VALUES (1, ?)]], {uuid1}) + +test:do_execsql_test( + "gh-6164-5", + [[ + SELECT GREATEST(i, s, x'33', 'something') FROM t; + ]], { + uuid1 + }) + +test:do_execsql_test( + "gh-6164-6", + [[ + SELECT LEAST(i, s, x'33', 'something') FROM t; + ]], { + 1 + }) + +box.execute([[INSERT INTO t VALUES (2, 2);]]) + +test:do_execsql_test( + "gh-6164-7", + [[ + SELECT MAX(s) FROM t; + ]], { + uuid1 + }) + +test:do_execsql_test( + "gh-6164-8", + [[ + SELECT MIN(s) FROM t; + ]], { + 2 + }) + +box.execute([[DROP TABLE t;]]) + test:finish_test() ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() 2021-07-11 17:51 ` Mergen Imeev via Tarantool-patches @ 2021-07-12 21:06 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-13 8:04 ` Mergen Imeev via Tarantool-patches 0 siblings, 1 reply; 15+ messages in thread From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-12 21:06 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches Hi! Thanks for the fixes! >>> +static inline enum mem_class >>> +mem_type_class(enum mem_type type) >>> +{ >>> + switch (type) { >>> + case MEM_TYPE_NULL: >>> + return MEM_CLASS_NULL; >>> + case MEM_TYPE_UINT: >>> + case MEM_TYPE_INT: >>> + case MEM_TYPE_DOUBLE: >>> + return MEM_CLASS_NUMBER; >>> + case MEM_TYPE_STR: >>> + return MEM_CLASS_STR; >>> + case MEM_TYPE_BIN: >>> + return MEM_CLASS_BIN; >>> + case MEM_TYPE_BOOL: >>> + return MEM_CLASS_BOOL; >>> + case MEM_TYPE_UUID: >>> + return MEM_CLASS_UUID; >> >> 3. It might work faster without branching if done like >> 'static enum mp_class mp_classes[]' - would allow to take >> the class for any type as simple as an array access >> operation. > This cannot be done directly in current design since value in enum mem_type are > not sequential numbers. However, this can be done using something like this: Ok, I see now. This is because I asked to make them bit fields before, shame on me. There is another option - use `32 - bit_ctz_u32()` to get a number from [1, 13] range and use it as an index in a small array. The problem here is that this is a bit crazy and I can't be sure if it works faster than this switch-case on all platforms. Hence, lets leave it as is now then. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 32d02d96e..220e8b269 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1828,7 +1828,7 @@ case OP_Compare: { > assert(i < (int)def->part_count); > struct coll *coll = def->parts[i].coll; > bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC; > - iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll); > + mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll); Why don't you check the result here? AFAIU, previously sqlMemCompare() never returned something undefined. Now iCompare might be left garbage leading to unstable behaviour. The same below. > if (iCompare) { > if (is_rev) > iCompare = -iCompare; > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index e5f35fbf8..dadc6d4a2 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ > rc = sql_stat4_column(db, samples[i].sample_key, nEq, > &pVal); > if (rc == 0 && p1 != NULL) { > - int res = sqlMemCompare(p1, pVal, coll); > + int res; > + mem_cmp_scalar(p1, pVal, &res, coll); > if (res >= 0) > nLower++; > } > if (rc == 0 && p2 != NULL) { > - int res = sqlMemCompare(p2, pVal, coll); > + int res; > + mem_cmp_scalar(p2, pVal, &res, coll); > if (res >= 0) > nUpper++; > } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() 2021-07-12 21:06 ` Vladislav Shpilevoy via Tarantool-patches @ 2021-07-13 8:04 ` Mergen Imeev via Tarantool-patches 0 siblings, 0 replies; 15+ messages in thread From: Mergen Imeev via Tarantool-patches @ 2021-07-13 8:04 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thank you for the review! My answer, diff and new patch below. On Mon, Jul 12, 2021 at 11:06:03PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > >>> +static inline enum mem_class > >>> +mem_type_class(enum mem_type type) > >>> +{ > >>> + switch (type) { > >>> + case MEM_TYPE_NULL: > >>> + return MEM_CLASS_NULL; > >>> + case MEM_TYPE_UINT: > >>> + case MEM_TYPE_INT: > >>> + case MEM_TYPE_DOUBLE: > >>> + return MEM_CLASS_NUMBER; > >>> + case MEM_TYPE_STR: > >>> + return MEM_CLASS_STR; > >>> + case MEM_TYPE_BIN: > >>> + return MEM_CLASS_BIN; > >>> + case MEM_TYPE_BOOL: > >>> + return MEM_CLASS_BOOL; > >>> + case MEM_TYPE_UUID: > >>> + return MEM_CLASS_UUID; > >> > >> 3. It might work faster without branching if done like > >> 'static enum mp_class mp_classes[]' - would allow to take > >> the class for any type as simple as an array access > >> operation. > > This cannot be done directly in current design since value in enum mem_type are > > not sequential numbers. However, this can be done using something like this: > > Ok, I see now. This is because I asked to make them bit fields > before, shame on me. There is another option - use `32 - bit_ctz_u32()` > to get a number from [1, 13] range and use it as an index in a small > array. The problem here is that this is a bit crazy and I can't be sure > if it works faster than this switch-case on all platforms. Hence, lets > leave it as is now then. > > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > > index 32d02d96e..220e8b269 100644 > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -1828,7 +1828,7 @@ case OP_Compare: { > > assert(i < (int)def->part_count); > > struct coll *coll = def->parts[i].coll; > > bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC; > > - iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll); > > + mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll); > > Why don't you check the result here? AFAIU, previously > sqlMemCompare() never returned something undefined. Now iCompare > might be left garbage leading to unstable behaviour. The same > below. > Fixed. Sorry, I didn't think about this last time, I should have fixed this in all places. > > if (iCompare) { > > if (is_rev) > > iCompare = -iCompare; > > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > > index e5f35fbf8..dadc6d4a2 100644 > > --- a/src/box/sql/where.c > > +++ b/src/box/sql/where.c > > @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ > > rc = sql_stat4_column(db, samples[i].sample_key, nEq, > > &pVal); > > if (rc == 0 && p1 != NULL) { > > - int res = sqlMemCompare(p1, pVal, coll); > > + int res; > > + mem_cmp_scalar(p1, pVal, &res, coll); > > if (res >= 0) > > nLower++; > > } > > if (rc == 0 && p2 != NULL) { > > - int res = sqlMemCompare(p2, pVal, coll); > > + int res; > > + mem_cmp_scalar(p2, pVal, &res, coll); > > if (res >= 0) > > nUpper++; > > } Diff: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 220e8b269..d322caef9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1828,7 +1828,13 @@ case OP_Compare: { assert(i < (int)def->part_count); struct coll *coll = def->parts[i].coll; bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC; - mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll); + struct Mem *a = &aMem[p1+idx]; + struct Mem *b = &aMem[p2+idx]; + if (mem_cmp_scalar(a, b, &iCompare, coll) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(b), + mem_type_to_str(a)); + goto abort_due_to_error; + } if (iCompare) { if (is_rev) iCompare = -iCompare; diff --git a/src/box/sql/where.c b/src/box/sql/where.c index dadc6d4a2..e2eb153fb 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -1273,14 +1273,26 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ &pVal); if (rc == 0 && p1 != NULL) { int res; - mem_cmp_scalar(p1, pVal, &res, coll); - if (res >= 0) + rc = mem_cmp_scalar(p1, pVal, &res, coll); + if (rc != 0) { + diag_set(ClientError, + ER_SQL_TYPE_MISMATCH, + mem_str(pVal), + mem_type_to_str(p1)); + } + if (rc == 0 && res >= 0) nLower++; } if (rc == 0 && p2 != NULL) { int res; - mem_cmp_scalar(p2, pVal, &res, coll); - if (res >= 0) + rc = mem_cmp_scalar(p2, pVal, &res, coll); + if (rc != 0) { + diag_set(ClientError, + ER_SQL_TYPE_MISMATCH, + mem_str(pVal), + mem_type_to_str(p2)); + } + if (rc == 0 && res >= 0) nUpper++; } } New patch: commit e17481d82926fb47b60eb6aefd2fa2110c1baa09 Author: Mergen Imeev <imeevma@gmail.com> Date: Sat Jul 10 13:56:39 2021 +0300 sql: introduce mem_cmp_scalar() This patch introduces the mem_cmp_scalar() function that compares two MEMs using SCALAR rules. MEMs must be scalars. Prior to this patch, there was a function that used SCALAR rules to compare two MEMs, but its design became overly complex as new types appeared. Part of #6164 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index aa565277c..efb14f23e 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -144,11 +144,16 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv) for (i = 1; i < argc; i++) { if (mem_is_null(argv[i])) return; - if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >= - 0) { - testcase(mask == 0); - iBest = i; + int res; + if (mem_cmp_scalar(argv[iBest], argv[i], &res, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(argv[i]), + mem_type_to_str(argv[iBest])); + context->is_aborted = true; + return; } + if ((res ^ mask) >= 0) + iBest = i; } sql_result_value(context, argv[iBest]); } @@ -1057,9 +1062,15 @@ nullifFunc(sql_context * context, int NotUsed, sql_value ** argv) { struct coll *pColl = sqlGetFuncCollSeq(context); UNUSED_PARAMETER(NotUsed); - if (sqlMemCompare(argv[0], argv[1], pColl) != 0) { - sql_result_value(context, argv[0]); + int res; + if (mem_cmp_scalar(argv[0], argv[1], &res, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(argv[1]), mem_type_to_str(argv[0])); + context->is_aborted = true; + return; } + if (res != 0) + sql_result_value(context, argv[0]); } /** @@ -1827,7 +1838,12 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) * comparison is inverted. */ bool is_max = (func->flags & SQL_FUNC_MAX) != 0; - cmp = sqlMemCompare(pBest, pArg, pColl); + if (mem_cmp_scalar(pBest, pArg, &cmp, pColl) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, + mem_str(pArg), mem_type_to_str(pBest)); + context->is_aborted = true; + return; + } if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { mem_copy(pBest, pArg); } else { diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 2595e2fd4..da27cd191 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -59,6 +59,44 @@ enum { BUF_SIZE = 32, }; +/** + * Analogue of enum mp_class for enum mp_type. The order of the classes must be + * the same as in the enum mp_class. + */ +enum mem_class { + MEM_CLASS_NULL, + MEM_CLASS_BOOL, + MEM_CLASS_NUMBER, + MEM_CLASS_STR, + MEM_CLASS_BIN, + MEM_CLASS_UUID, + mem_class_max, +}; + +static inline enum mem_class +mem_type_class(enum mem_type type) +{ + switch (type) { + case MEM_TYPE_NULL: + return MEM_CLASS_NULL; + case MEM_TYPE_UINT: + case MEM_TYPE_INT: + case MEM_TYPE_DOUBLE: + return MEM_CLASS_NUMBER; + case MEM_TYPE_STR: + return MEM_CLASS_STR; + case MEM_TYPE_BIN: + return MEM_CLASS_BIN; + case MEM_TYPE_BOOL: + return MEM_CLASS_BOOL; + case MEM_TYPE_UUID: + return MEM_CLASS_UUID; + default: + break; + } + return mem_class_max; +} + bool mem_is_field_compatible(const struct Mem *mem, enum field_type type) { @@ -2009,6 +2047,36 @@ mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result) return 0; } +int +mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, + const struct coll *coll) +{ + enum mem_class class_a = mem_type_class(a->type); + enum mem_class class_b = mem_type_class(b->type); + if (class_a != class_b) { + *result = class_a - class_b; + return 0; + } + switch (class_a) { + case MEM_CLASS_NULL: + *result = 0; + return 0; + case MEM_CLASS_BOOL: + return mem_cmp_bool(a, b, result); + case MEM_CLASS_NUMBER: + return mem_cmp_num(a, b, result); + case MEM_CLASS_STR: + return mem_cmp_str(a, b, result, coll); + case MEM_CLASS_BIN: + return mem_cmp_bin(a, b, result); + case MEM_CLASS_UUID: + return mem_cmp_uuid(a, b, result); + default: + unreachable(); + } + return 0; +} + /* * Both *pMem1 and *pMem2 contain string values. Compare the two values * using the collation sequence pColl. As usual, return a negative , zero @@ -2440,82 +2508,6 @@ sqlVdbeMemTooBig(Mem * p) return 0; } -/* - * Compare the values contained by the two memory cells, returning - * negative, zero or positive if pMem1 is less than, equal to, or greater - * than pMem2. Sorting order is NULL's first, followed by numbers (integers - * and reals) sorted numerically, followed by text ordered by the collating - * sequence pColl and finally blob's ordered by memcmp(). - * - * Two NULL values are considered equal by this function. - */ -int -sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl) -{ - int res; - - enum mem_type type1 = pMem1->type; - enum mem_type type2 = pMem2->type; - - /* If one value is NULL, it is less than the other. If both values - * are NULL, return 0. - */ - if (((type1 | type2) & MEM_TYPE_NULL) != 0) - return (int)(type2 == MEM_TYPE_NULL) - - (int)(type1 == MEM_TYPE_NULL); - - if (((type1 | type2) & MEM_TYPE_BOOL) != 0) { - if ((type1 & type2 & MEM_TYPE_BOOL) != 0) { - if (pMem1->u.b == pMem2->u.b) - return 0; - if (pMem1->u.b) - return 1; - return -1; - } - if (type2 == MEM_TYPE_BOOL) - return +1; - return -1; - } - - if (((type1 | type2) & MEM_TYPE_UUID) != 0) { - if (mem_cmp_uuid(pMem1, pMem2, &res) == 0) - return res; - if (type1 != MEM_TYPE_UUID) - return +1; - return -1; - } - - /* At least one of the two values is a number - */ - if (((type1 | type2) & - (MEM_TYPE_INT | MEM_TYPE_UINT | MEM_TYPE_DOUBLE)) != 0) { - if (!mem_is_num(pMem1)) - return +1; - if (!mem_is_num(pMem2)) - return -1; - mem_cmp_num(pMem1, pMem2, &res); - return res; - } - - /* If one value is a string and the other is a blob, the string is less. - * If both are strings, compare using the collating functions. - */ - if (((type1 | type2) & MEM_TYPE_STR) != 0) { - if (type1 != MEM_TYPE_STR) { - return 1; - } - if (type2 != MEM_TYPE_STR) { - return -1; - } - mem_cmp_str(pMem1, pMem2, &res, pColl); - return res; - } - - /* Both values must be blobs. Compare using memcmp(). */ - mem_cmp_bin(pMem1, pMem2, &res); - return res; -} - int sql_vdbemem_finalize(struct Mem *mem, struct func *func) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index b3cd5c545..bbb99c4d2 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -696,6 +696,14 @@ mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result); int mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result); +/** + * Compare two MEMs using SCALAR rules and return the result of comparison. MEMs + * should be scalars. Original MEMs are not changed. + */ +int +mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, + const struct coll *coll); + /** * Convert the given MEM to INTEGER. This function and the function below define * the rules that are used to convert values of all other types to INTEGER. In @@ -961,8 +969,6 @@ int sqlVdbeMemTooBig(Mem *); #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\ ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0) -int sqlMemCompare(const Mem *, const Mem *, const struct coll *); - /** MEM manipulate functions. */ /** diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 32d02d96e..d322caef9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1828,7 +1828,13 @@ case OP_Compare: { assert(i < (int)def->part_count); struct coll *coll = def->parts[i].coll; bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC; - iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll); + struct Mem *a = &aMem[p1+idx]; + struct Mem *b = &aMem[p2+idx]; + if (mem_cmp_scalar(a, b, &iCompare, coll) != 0) { + diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(b), + mem_type_to_str(a)); + goto abort_due_to_error; + } if (iCompare) { if (is_rev) iCompare = -iCompare; diff --git a/src/box/sql/where.c b/src/box/sql/where.c index e5f35fbf8..e2eb153fb 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -1272,13 +1272,27 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ rc = sql_stat4_column(db, samples[i].sample_key, nEq, &pVal); if (rc == 0 && p1 != NULL) { - int res = sqlMemCompare(p1, pVal, coll); - if (res >= 0) + int res; + rc = mem_cmp_scalar(p1, pVal, &res, coll); + if (rc != 0) { + diag_set(ClientError, + ER_SQL_TYPE_MISMATCH, + mem_str(pVal), + mem_type_to_str(p1)); + } + if (rc == 0 && res >= 0) nLower++; } if (rc == 0 && p2 != NULL) { - int res = sqlMemCompare(p2, pVal, coll); - if (res >= 0) + int res; + rc = mem_cmp_scalar(p2, pVal, &res, coll); + if (rc != 0) { + diag_set(ClientError, + ER_SQL_TYPE_MISMATCH, + mem_str(pVal), + mem_type_to_str(p2)); + } + if (rc == 0 && res >= 0) nUpper++; } } diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua index 4fc5052d8..6b4a811c3 100755 --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(4) +test:plan(8) -- Make sure that function quote() can work with uuid. test:do_execsql_test( @@ -35,4 +35,47 @@ test:do_test( end, uuid3) +-- +-- Make sure a comparison that includes a UUID and follows the SCALAR rules is +-- working correctly. +-- +box.execute([[CREATE TABLE t (i INTEGER PRIMARY KEY, s SCALAR);]]) +box.execute([[INSERT INTO t VALUES (1, ?)]], {uuid1}) + +test:do_execsql_test( + "gh-6164-5", + [[ + SELECT GREATEST(i, s, x'33', 'something') FROM t; + ]], { + uuid1 + }) + +test:do_execsql_test( + "gh-6164-6", + [[ + SELECT LEAST(i, s, x'33', 'something') FROM t; + ]], { + 1 + }) + +box.execute([[INSERT INTO t VALUES (2, 2);]]) + +test:do_execsql_test( + "gh-6164-7", + [[ + SELECT MAX(s) FROM t; + ]], { + uuid1 + }) + +test:do_execsql_test( + "gh-6164-8", + [[ + SELECT MIN(s) FROM t; + ]], { + 2 + }) + +box.execute([[DROP TABLE t;]]) + test:finish_test() ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-19 10:07 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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-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 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-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 2021-07-19 9:16 ` Timur Safin via Tarantool-patches 2021-07-19 10:07 ` Mergen Imeev via Tarantool-patches -- strict thread matches above, loose matches on Subject: below -- 2021-07-10 14:33 [Tarantool-patches] [PATCH v2 0/4] Follow ups for uuid introduction Mergen Imeev via Tarantool-patches 2021-07-10 14:33 ` [Tarantool-patches] [PATCH v2 3/4] sql: introduce mem_cmp_scalar() Mergen Imeev via Tarantool-patches 2021-07-11 15:03 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-11 17:51 ` Mergen Imeev via Tarantool-patches 2021-07-12 21:06 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-13 8:04 ` Mergen Imeev via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox