* [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
* [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
* [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
* [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 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
* 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
* 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
* 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
* 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
* 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-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-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
* [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
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