This patch introduces the mem_cmp_scalar() function that compares MEM and packed to msgpack value using SCALAR rules. MEM and packed value must be scalars. Prior to this patch, there was a function that used SCALAR rules to compare MEM and packed value, but its design became overly complex as new types appeared. Closes #6164 --- src/box/sql.c | 9 +- src/box/sql/mem.c | 282 +++++------------- src/box/sql/mem.h | 23 +- test/sql-tap/gh-6164-uuid-follow-ups.test.lua | 14 +- 4 files changed, 100 insertions(+), 228 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index 790ca7f70..7f24bd778 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -765,13 +765,16 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, } } next_fieldno = fieldno + 1; - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); + struct key_part *part = &unpacked->key_def->parts[i]; + struct Mem *mem = unpacked->aMem + i; + struct coll *coll = part->coll; + mem_cmp_msgpack(mem, p, &rc, coll); if (rc != 0) { - if (unpacked->key_def->parts[i].sort_order != - SORT_ORDER_ASC) + if (part->sort_order == SORT_ORDER_ASC) rc = -rc; goto out; } + mp_next(&p); } rc = unpacked->default_rc; out: diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 576596c9f..471b69a18 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -2073,35 +2073,72 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, return 0; } -/* - * Both *pMem1 and *pMem2 contain string values. Compare the two values - * using the collation sequence pColl. As usual, return a negative , zero - * or positive value if *pMem1 is less than, equal to or greater than - * *pMem2, respectively. Similar in spirit to "rc = (*pMem1) - (*pMem2);". - * - * Strungs assume to be UTF-8 encoded - */ -static int -vdbeCompareMemString(const Mem * pMem1, const Mem * pMem2, - const struct coll * pColl) -{ - return pColl->cmp(pMem1->z, (size_t)pMem1->n, - pMem2->z, (size_t)pMem2->n, pColl); -} - -/* - * The input pBlob is guaranteed to be a Blob that is not marked - * with MEM_Zero. Return true if it could be a zero-blob. - */ -static int -isAllZero(const char *z, int n) +int +mem_cmp_msgpack(const struct Mem *a, const char *b, int *result, + const struct coll *coll) { - int i; - for (i = 0; i < n; i++) { - if (z[i]) - return 0; + struct Mem mem; + switch (mp_typeof(*b)) { + case MP_NIL: + mem.type = MEM_TYPE_NULL; + break; + case MP_BOOL: + mem.type = MEM_TYPE_BOOL; + mem.u.b = mp_decode_bool(&b); + break; + case MP_UINT: + mem.type = MEM_TYPE_UINT; + mem.u.u = mp_decode_uint(&b); + break; + case MP_INT: + mem.type = MEM_TYPE_INT; + mem.u.i = mp_decode_int(&b); + break; + case MP_FLOAT: + mem.type = MEM_TYPE_DOUBLE; + mem.u.r = mp_decode_float(&b); + break; + case MP_DOUBLE: + mem.type = MEM_TYPE_DOUBLE; + mem.u.r = mp_decode_double(&b); + break; + case MP_STR: + mem.type = MEM_TYPE_STR; + mem.n = mp_decode_strl(&b); + mem.z = (char *)b; + mem.flags = MEM_Ephem; + break; + case MP_BIN: + mem.type = MEM_TYPE_BIN; + mem.n = mp_decode_binl(&b); + mem.z = (char *)b; + break; + case MP_ARRAY: + case MP_MAP: + *result = -1; + return 0; + case MP_EXT: { + int8_t type; + const char *buf = b; + uint32_t len = mp_decode_extl(&b, &type); + if (type == MP_UUID) { + assert(len == UUID_LEN); + mem.type = MEM_TYPE_UUID; + b = buf; + if (mp_decode_uuid(&b, &mem.u.uuid) == NULL) + return -1; + break; + } + b += len; + mem.type = MEM_TYPE_BIN; + mem.z = (char *)buf; + mem.n = b - buf; + break; } - return 1; + default: + unreachable(); + } + return mem_cmp_scalar(a, &mem, result, coll); } char * @@ -2530,183 +2567,6 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func) return ctx.is_aborted ? -1 : 0; } -int -sqlVdbeCompareMsgpack(const char **key1, - struct UnpackedRecord *unpacked, int key2_idx) -{ - const char *aKey1 = *key1; - Mem *pKey2 = unpacked->aMem + key2_idx; - Mem mem1; - int rc = 0; - switch (mp_typeof(*aKey1)) { - default:{ - /* FIXME */ - rc = -1; - break; - } - case MP_NIL:{ - rc = -(pKey2->type != MEM_TYPE_NULL); - mp_decode_nil(&aKey1); - break; - } - case MP_BOOL:{ - mem1.u.b = mp_decode_bool(&aKey1); - if (pKey2->type == MEM_TYPE_BOOL) { - if (mem1.u.b != pKey2->u.b) - rc = mem1.u.b ? 1 : -1; - } else { - rc = pKey2->type == MEM_TYPE_NULL ? 1 : -1; - } - break; - } - case MP_UINT:{ - mem1.u.u = mp_decode_uint(&aKey1); - if (pKey2->type == MEM_TYPE_INT) { - rc = +1; - } else if (pKey2->type == MEM_TYPE_UINT) { - if (mem1.u.u < pKey2->u.u) - rc = -1; - else if (mem1.u.u > pKey2->u.u) - rc = +1; - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - rc = double_compare_uint64(pKey2->u.r, - mem1.u.u, -1); - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_INT:{ - mem1.u.i = mp_decode_int(&aKey1); - if (pKey2->type == MEM_TYPE_UINT) { - rc = -1; - } else if (pKey2->type == MEM_TYPE_INT) { - if (mem1.u.i < pKey2->u.i) { - rc = -1; - } else if (mem1.u.i > pKey2->u.i) { - rc = +1; - } - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - rc = double_compare_nint64(pKey2->u.r, mem1.u.i, - -1); - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_FLOAT:{ - mem1.u.r = mp_decode_float(&aKey1); - goto do_float; - } - case MP_DOUBLE:{ - mem1.u.r = mp_decode_double(&aKey1); - do_float: - if (pKey2->type == MEM_TYPE_INT) { - rc = double_compare_nint64(mem1.u.r, pKey2->u.i, - 1); - } else if (pKey2->type == MEM_TYPE_UINT) { - rc = double_compare_uint64(mem1.u.r, - pKey2->u.u, 1); - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - if (mem1.u.r < pKey2->u.r) { - rc = -1; - } else if (mem1.u.r > pKey2->u.r) { - rc = +1; - } - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_STR:{ - if (pKey2->type == MEM_TYPE_STR) { - struct key_def *key_def = unpacked->key_def; - mem1.n = mp_decode_strl(&aKey1); - mem1.z = (char *)aKey1; - aKey1 += mem1.n; - struct coll *coll = - key_def->parts[key2_idx].coll; - if (coll != NULL) { - mem1.type = MEM_TYPE_STR; - mem1.flags = 0; - rc = vdbeCompareMemString(&mem1, pKey2, - coll); - } else { - goto do_bin_cmp; - } - } else { - rc = pKey2->type == MEM_TYPE_BIN ? -1 : +1; - } - break; - } - case MP_BIN:{ - mem1.n = mp_decode_binl(&aKey1); - mem1.z = (char *)aKey1; - aKey1 += mem1.n; - do_blob: - if (pKey2->type == MEM_TYPE_BIN) { - if (pKey2->flags & MEM_Zero) { - if (!isAllZero - ((const char *)mem1.z, mem1.n)) { - rc = 1; - } else { - rc = mem1.n - pKey2->u.nZero; - } - } else { - int nCmp; - do_bin_cmp: - nCmp = MIN(mem1.n, pKey2->n); - rc = memcmp(mem1.z, pKey2->z, nCmp); - if (rc == 0) - rc = mem1.n - pKey2->n; - } - } else { - rc = 1; - } - break; - } - case MP_ARRAY: - case MP_MAP: { - mem1.z = (char *)aKey1; - mp_next(&aKey1); - mem1.n = aKey1 - (char *)mem1.z; - goto do_blob; - } - case MP_EXT: { - int8_t type; - const char *buf = aKey1; - uint32_t len = mp_decode_extl(&aKey1, &type); - if (type == MP_UUID) { - assert(len == UUID_LEN); - mem1.type = MEM_TYPE_UUID; - aKey1 = buf; - if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL || - mem_cmp_uuid(&mem1, pKey2, &rc) != 0) - rc = 1; - break; - } - aKey1 += len; - mem1.z = (char *)buf; - mem1.n = aKey1 - buf; - goto do_blob; - } - } - *key1 = aKey1; - return rc; -} - int sqlVdbeRecordCompareMsgpack(const void *key1, struct UnpackedRecord *key2) @@ -2717,14 +2577,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1, n = MIN(n, key2->nField); for (i = 0; i != n; i++) { - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); + struct key_part *part = &key2->key_def->parts[i]; + struct Mem *mem = key2->aMem + i; + struct coll *coll = part->coll; + mem_cmp_msgpack(mem, key1, &rc, coll); if (rc != 0) { - if (key2->key_def->parts[i].sort_order != - SORT_ORDER_ASC) { - rc = -rc; - } - return rc; + if (part->sort_order != SORT_ORDER_ASC) + return rc; + return -rc; } + mp_next((const char **)&key1); } key2->eqSeen = 1; diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index bbb99c4d2..2439fd93b 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -704,6 +704,15 @@ int mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, const struct coll *coll); +/** + * Compare MEM and packed to msgpack value using SCALAR rules and return the + * result of comparison. Both values should be scalars. Original MEM and packed + * value are not changed. + */ +int +mem_cmp_msgpack(const struct Mem *a, const char *b, int *result, + const struct coll *coll); + /** * Convert the given MEM to INTEGER. This function and the function below define * the rules that are used to convert values of all other types to INTEGER. In @@ -981,20 +990,6 @@ int sqlVdbeMemTooBig(Mem *); int sql_vdbemem_finalize(struct Mem *mem, struct func *func); -/** MEM and msgpack functions. */ - -/** - * Perform comparison of two keys: one is packed and one is not. - * - * @param key1 Pointer to pointer to first key. - * @param unpacked Pointer to unpacked tuple. - * @param key2_idx index of key in umpacked record to compare. - * - * @retval +1 if key1 > pUnpacked[iKey2], -1 ptherwise. - */ -int sqlVdbeCompareMsgpack(const char **key1, - struct UnpackedRecord *unpacked, int key2_idx); - /** * Perform comparison of two tuples: unpacked (key1) and packed (key2) * diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua index 6b4a811c3..dd041c0b4 100755 --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(8) +test:plan(9) -- Make sure that function quote() can work with uuid. test:do_execsql_test( @@ -76,6 +76,18 @@ test:do_execsql_test( 2 }) +box.execute([[DELETE FROM t;]]) + +box.execute([[INSERT INTO t VALUES (1, X'00'), (2, ?);]], {uuid1}) + +test:do_execsql_test( + "gh-6164-9", + [[ + SELECT i FROM t ORDER BY s; + ]], { + 1, 2 + }) + box.execute([[DROP TABLE t;]]) test:finish_test() -- 2.25.1
Thanks for the patch! See 4 comments below. On 10.07.2021 16:33, Mergen Imeev via Tarantool-patches wrote: > This patch introduces the mem_cmp_scalar() function that compares MEM 1. scalar -> msgpack. > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index 576596c9f..471b69a18 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -2073,35 +2073,72 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, <...> > - > -/* > - * The input pBlob is guaranteed to be a Blob that is not marked > - * with MEM_Zero. Return true if it could be a zero-blob. > - */ > -static int > -isAllZero(const char *z, int n) > +int > +mem_cmp_msgpack(const struct Mem *a, const char *b, int *result, > + const struct coll *coll) 2. Maybe better make the function return 'b' after decoding via making it an out parameter. So as the caller could save mp_next() call. > { > - int i; > - for (i = 0; i < n; i++) { > - if (z[i]) > - return 0; > + struct Mem mem; > + switch (mp_typeof(*b)) { > + case MP_NIL: > + mem.type = MEM_TYPE_NULL; > + break; > + case MP_BOOL: > + mem.type = MEM_TYPE_BOOL; > + mem.u.b = mp_decode_bool(&b); > + break; > + case MP_UINT: > + mem.type = MEM_TYPE_UINT; > + mem.u.u = mp_decode_uint(&b); > + break; > + case MP_INT: > + mem.type = MEM_TYPE_INT; > + mem.u.i = mp_decode_int(&b); > + break; > + case MP_FLOAT: > + mem.type = MEM_TYPE_DOUBLE; > + mem.u.r = mp_decode_float(&b); > + break; > + case MP_DOUBLE: > + mem.type = MEM_TYPE_DOUBLE; > + mem.u.r = mp_decode_double(&b); > + break; > + case MP_STR: > + mem.type = MEM_TYPE_STR; > + mem.n = mp_decode_strl(&b); > + mem.z = (char *)b; > + mem.flags = MEM_Ephem; > + break; > + case MP_BIN: > + mem.type = MEM_TYPE_BIN; > + mem.n = mp_decode_binl(&b); > + mem.z = (char *)b; 3. Shouldn't this also have MEM_Ephem? > + break; > + case MP_ARRAY: > + case MP_MAP: > + *result = -1; > + return 0; > + case MP_EXT: { > + int8_t type; > + const char *buf = b; > + uint32_t len = mp_decode_extl(&b, &type); > + if (type == MP_UUID) { > + assert(len == UUID_LEN); > + mem.type = MEM_TYPE_UUID; > + b = buf; 4. You can decode &buf below without changing b. > + if (mp_decode_uuid(&b, &mem.u.uuid) == NULL) > + return -1; > + break; > + } > + b += len; > + mem.type = MEM_TYPE_BIN; > + mem.z = (char *)buf; > + mem.n = b - buf; > + break; > } > - return 1; > + default: > + unreachable(); > + } > + return mem_cmp_scalar(a, &mem, result, coll); > }
Thank you for the review! My answers, diff and new patch below. On Sun, Jul 11, 2021 at 05:05:38PM +0200, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 4 comments below. > > On 10.07.2021 16:33, Mergen Imeev via Tarantool-patches wrote: > > This patch introduces the mem_cmp_scalar() function that compares MEM > > 1. scalar -> msgpack. > Thanks, fixed. > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index 576596c9f..471b69a18 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -2073,35 +2073,72 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, > > <...> > > > - > > -/* > > - * The input pBlob is guaranteed to be a Blob that is not marked > > - * with MEM_Zero. Return true if it could be a zero-blob. > > - */ > > -static int > > -isAllZero(const char *z, int n) > > +int > > +mem_cmp_msgpack(const struct Mem *a, const char *b, int *result, > > + const struct coll *coll) > > 2. Maybe better make the function return 'b' after decoding via > making it an out parameter. So as the caller could save mp_next() > call. > Agree. Done. Also, I could use here mp_decode_str()/mp_decode_bin() instead of mp_decode_strl()/mp_decode_binl() but decided to do this when I will change struct Mem according to discussion. Hope, I won't forget about this. > > { > > - int i; > > - for (i = 0; i < n; i++) { > > - if (z[i]) > > - return 0; > > + struct Mem mem; > > + switch (mp_typeof(*b)) { > > + case MP_NIL: > > + mem.type = MEM_TYPE_NULL; > > + break; > > + case MP_BOOL: > > + mem.type = MEM_TYPE_BOOL; > > + mem.u.b = mp_decode_bool(&b); > > + break; > > + case MP_UINT: > > + mem.type = MEM_TYPE_UINT; > > + mem.u.u = mp_decode_uint(&b); > > + break; > > + case MP_INT: > > + mem.type = MEM_TYPE_INT; > > + mem.u.i = mp_decode_int(&b); > > + break; > > + case MP_FLOAT: > > + mem.type = MEM_TYPE_DOUBLE; > > + mem.u.r = mp_decode_float(&b); > > + break; > > + case MP_DOUBLE: > > + mem.type = MEM_TYPE_DOUBLE; > > + mem.u.r = mp_decode_double(&b); > > + break; > > + case MP_STR: > > + mem.type = MEM_TYPE_STR; > > + mem.n = mp_decode_strl(&b); > > + mem.z = (char *)b; > > + mem.flags = MEM_Ephem; > > + break; > > + case MP_BIN: > > + mem.type = MEM_TYPE_BIN; > > + mem.n = mp_decode_binl(&b); > > + mem.z = (char *)b; > > 3. Shouldn't this also have MEM_Ephem? > Fixed here and below. > > + break; > > + case MP_ARRAY: > > + case MP_MAP: > > + *result = -1; > > + return 0; > > + case MP_EXT: { > > + int8_t type; > > + const char *buf = b; > > + uint32_t len = mp_decode_extl(&b, &type); > > + if (type == MP_UUID) { > > + assert(len == UUID_LEN); > > + mem.type = MEM_TYPE_UUID; > > + b = buf; > > 4. You can decode &buf below without changing b. > I changed a bit this part. > > + if (mp_decode_uuid(&b, &mem.u.uuid) == NULL) > > + return -1; > > + break; > > + } > > + b += len; > > + mem.type = MEM_TYPE_BIN; > > + mem.z = (char *)buf; > > + mem.n = b - buf; > > + break; > > } > > - return 1; > > + default: > > + unreachable(); > > + } > > + return mem_cmp_scalar(a, &mem, result, coll); > > } Diff: diff --git a/src/box/sql.c b/src/box/sql.c index 7f24bd778..a5afcfabb 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -768,13 +768,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, struct key_part *part = &unpacked->key_def->parts[i]; struct Mem *mem = unpacked->aMem + i; struct coll *coll = part->coll; - mem_cmp_msgpack(mem, p, &rc, coll); + mem_cmp_msgpack(mem, &p, &rc, coll); if (rc != 0) { if (part->sort_order == SORT_ORDER_ASC) rc = -rc; goto out; } - mp_next(&p); } rc = unpacked->default_rc; out: diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 5e7b1f28b..4062ff4b3 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -2078,65 +2078,71 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, } int -mem_cmp_msgpack(const struct Mem *a, const char *b, int *result, +mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, const struct coll *coll) { struct Mem mem; - switch (mp_typeof(*b)) { + switch (mp_typeof(**b)) { case MP_NIL: mem.type = MEM_TYPE_NULL; + mp_decode_nil(b); break; case MP_BOOL: mem.type = MEM_TYPE_BOOL; - mem.u.b = mp_decode_bool(&b); + mem.u.b = mp_decode_bool(b); break; case MP_UINT: mem.type = MEM_TYPE_UINT; - mem.u.u = mp_decode_uint(&b); + mem.u.u = mp_decode_uint(b); break; case MP_INT: mem.type = MEM_TYPE_INT; - mem.u.i = mp_decode_int(&b); + mem.u.i = mp_decode_int(b); break; case MP_FLOAT: mem.type = MEM_TYPE_DOUBLE; - mem.u.r = mp_decode_float(&b); + mem.u.r = mp_decode_float(b); break; case MP_DOUBLE: mem.type = MEM_TYPE_DOUBLE; - mem.u.r = mp_decode_double(&b); + mem.u.r = mp_decode_double(b); break; case MP_STR: mem.type = MEM_TYPE_STR; - mem.n = mp_decode_strl(&b); - mem.z = (char *)b; + mem.n = mp_decode_strl(b); + mem.z = (char *)*b; + *b += mem.n; mem.flags = MEM_Ephem; break; case MP_BIN: mem.type = MEM_TYPE_BIN; - mem.n = mp_decode_binl(&b); - mem.z = (char *)b; + mem.n = mp_decode_binl(b); + mem.z = (char *)*b; + *b += mem.n; + mem.flags = MEM_Ephem; break; case MP_ARRAY: case MP_MAP: + mp_next(b); *result = -1; return 0; case MP_EXT: { int8_t type; - const char *buf = b; - uint32_t len = mp_decode_extl(&b, &type); + const char *buf = *b; + uint32_t len = mp_decode_extl(&buf, &type); if (type == MP_UUID) { assert(len == UUID_LEN); mem.type = MEM_TYPE_UUID; - b = buf; - if (mp_decode_uuid(&b, &mem.u.uuid) == NULL) + if (mp_decode_uuid(b, &mem.u.uuid) == NULL) return -1; break; } - b += len; + len += buf - *b; mem.type = MEM_TYPE_BIN; - mem.z = (char *)buf; - mem.n = b - buf; + mem.z = (char *)*b; + mem.n = len; + mem.flags = MEM_Ephem; + *b += len; break; } default: @@ -2584,13 +2590,12 @@ sqlVdbeRecordCompareMsgpack(const void *key1, struct key_part *part = &key2->key_def->parts[i]; struct Mem *mem = key2->aMem + i; struct coll *coll = part->coll; - mem_cmp_msgpack(mem, key1, &rc, coll); + mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll); if (rc != 0) { if (part->sort_order != SORT_ORDER_ASC) return rc; return -rc; } - mp_next((const char **)&key1); } key2->eqSeen = 1; diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 2439fd93b..c73536efb 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -706,11 +706,12 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, /** * Compare MEM and packed to msgpack value using SCALAR rules and return the - * result of comparison. Both values should be scalars. Original MEM and packed - * value are not changed. + * result of comparison. Both values should be scalars. Original MEM is not + * changed. If successful, the second argument will contain the address + * following the specified packed value. */ int -mem_cmp_msgpack(const struct Mem *a, const char *b, int *result, +mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, const struct coll *coll); /** New patch: commit aa422cd2f19813503664e7292dbd01e7b0711a7d Author: Mergen Imeev <imeevma@gmail.com> Date: Fri Jul 9 11:59:34 2021 +0300 sql: introduce mem_cmp_msgpack() This patch introduces the mem_cmp_msgpack() function that compares MEM and packed to msgpack value using SCALAR rules. MEM and packed value must be scalars. Prior to this patch, there was a function that used SCALAR rules to compare MEM and packed value, but its design became overly complex as new types appeared. Closes #6164 diff --git a/src/box/sql.c b/src/box/sql.c index 790ca7f70..a5afcfabb 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -765,10 +765,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, } } next_fieldno = fieldno + 1; - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); + struct key_part *part = &unpacked->key_def->parts[i]; + struct Mem *mem = unpacked->aMem + i; + struct coll *coll = part->coll; + mem_cmp_msgpack(mem, &p, &rc, coll); if (rc != 0) { - if (unpacked->key_def->parts[i].sort_order != - SORT_ORDER_ASC) + if (part->sort_order == SORT_ORDER_ASC) rc = -rc; goto out; } diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index da27cd191..4062ff4b3 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -2077,35 +2077,78 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, return 0; } -/* - * Both *pMem1 and *pMem2 contain string values. Compare the two values - * using the collation sequence pColl. As usual, return a negative , zero - * or positive value if *pMem1 is less than, equal to or greater than - * *pMem2, respectively. Similar in spirit to "rc = (*pMem1) - (*pMem2);". - * - * Strungs assume to be UTF-8 encoded - */ -static int -vdbeCompareMemString(const Mem * pMem1, const Mem * pMem2, - const struct coll * pColl) -{ - return pColl->cmp(pMem1->z, (size_t)pMem1->n, - pMem2->z, (size_t)pMem2->n, pColl); -} - -/* - * The input pBlob is guaranteed to be a Blob that is not marked - * with MEM_Zero. Return true if it could be a zero-blob. - */ -static int -isAllZero(const char *z, int n) -{ - int i; - for (i = 0; i < n; i++) { - if (z[i]) - return 0; +int +mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, + const struct coll *coll) +{ + struct Mem mem; + switch (mp_typeof(**b)) { + case MP_NIL: + mem.type = MEM_TYPE_NULL; + mp_decode_nil(b); + break; + case MP_BOOL: + mem.type = MEM_TYPE_BOOL; + mem.u.b = mp_decode_bool(b); + break; + case MP_UINT: + mem.type = MEM_TYPE_UINT; + mem.u.u = mp_decode_uint(b); + break; + case MP_INT: + mem.type = MEM_TYPE_INT; + mem.u.i = mp_decode_int(b); + break; + case MP_FLOAT: + mem.type = MEM_TYPE_DOUBLE; + mem.u.r = mp_decode_float(b); + break; + case MP_DOUBLE: + mem.type = MEM_TYPE_DOUBLE; + mem.u.r = mp_decode_double(b); + break; + case MP_STR: + mem.type = MEM_TYPE_STR; + mem.n = mp_decode_strl(b); + mem.z = (char *)*b; + *b += mem.n; + mem.flags = MEM_Ephem; + break; + case MP_BIN: + mem.type = MEM_TYPE_BIN; + mem.n = mp_decode_binl(b); + mem.z = (char *)*b; + *b += mem.n; + mem.flags = MEM_Ephem; + break; + case MP_ARRAY: + case MP_MAP: + mp_next(b); + *result = -1; + return 0; + case MP_EXT: { + int8_t type; + const char *buf = *b; + uint32_t len = mp_decode_extl(&buf, &type); + if (type == MP_UUID) { + assert(len == UUID_LEN); + mem.type = MEM_TYPE_UUID; + if (mp_decode_uuid(b, &mem.u.uuid) == NULL) + return -1; + break; + } + len += buf - *b; + mem.type = MEM_TYPE_BIN; + mem.z = (char *)*b; + mem.n = len; + mem.flags = MEM_Ephem; + *b += len; + break; } - return 1; + default: + unreachable(); + } + return mem_cmp_scalar(a, &mem, result, coll); } char * @@ -2534,183 +2577,6 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func) return ctx.is_aborted ? -1 : 0; } -int -sqlVdbeCompareMsgpack(const char **key1, - struct UnpackedRecord *unpacked, int key2_idx) -{ - const char *aKey1 = *key1; - Mem *pKey2 = unpacked->aMem + key2_idx; - Mem mem1; - int rc = 0; - switch (mp_typeof(*aKey1)) { - default:{ - /* FIXME */ - rc = -1; - break; - } - case MP_NIL:{ - rc = -(pKey2->type != MEM_TYPE_NULL); - mp_decode_nil(&aKey1); - break; - } - case MP_BOOL:{ - mem1.u.b = mp_decode_bool(&aKey1); - if (pKey2->type == MEM_TYPE_BOOL) { - if (mem1.u.b != pKey2->u.b) - rc = mem1.u.b ? 1 : -1; - } else { - rc = pKey2->type == MEM_TYPE_NULL ? 1 : -1; - } - break; - } - case MP_UINT:{ - mem1.u.u = mp_decode_uint(&aKey1); - if (pKey2->type == MEM_TYPE_INT) { - rc = +1; - } else if (pKey2->type == MEM_TYPE_UINT) { - if (mem1.u.u < pKey2->u.u) - rc = -1; - else if (mem1.u.u > pKey2->u.u) - rc = +1; - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - rc = double_compare_uint64(pKey2->u.r, - mem1.u.u, -1); - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_INT:{ - mem1.u.i = mp_decode_int(&aKey1); - if (pKey2->type == MEM_TYPE_UINT) { - rc = -1; - } else if (pKey2->type == MEM_TYPE_INT) { - if (mem1.u.i < pKey2->u.i) { - rc = -1; - } else if (mem1.u.i > pKey2->u.i) { - rc = +1; - } - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - rc = double_compare_nint64(pKey2->u.r, mem1.u.i, - -1); - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_FLOAT:{ - mem1.u.r = mp_decode_float(&aKey1); - goto do_float; - } - case MP_DOUBLE:{ - mem1.u.r = mp_decode_double(&aKey1); - do_float: - if (pKey2->type == MEM_TYPE_INT) { - rc = double_compare_nint64(mem1.u.r, pKey2->u.i, - 1); - } else if (pKey2->type == MEM_TYPE_UINT) { - rc = double_compare_uint64(mem1.u.r, - pKey2->u.u, 1); - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - if (mem1.u.r < pKey2->u.r) { - rc = -1; - } else if (mem1.u.r > pKey2->u.r) { - rc = +1; - } - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_STR:{ - if (pKey2->type == MEM_TYPE_STR) { - struct key_def *key_def = unpacked->key_def; - mem1.n = mp_decode_strl(&aKey1); - mem1.z = (char *)aKey1; - aKey1 += mem1.n; - struct coll *coll = - key_def->parts[key2_idx].coll; - if (coll != NULL) { - mem1.type = MEM_TYPE_STR; - mem1.flags = 0; - rc = vdbeCompareMemString(&mem1, pKey2, - coll); - } else { - goto do_bin_cmp; - } - } else { - rc = pKey2->type == MEM_TYPE_BIN ? -1 : +1; - } - break; - } - case MP_BIN:{ - mem1.n = mp_decode_binl(&aKey1); - mem1.z = (char *)aKey1; - aKey1 += mem1.n; - do_blob: - if (pKey2->type == MEM_TYPE_BIN) { - if (pKey2->flags & MEM_Zero) { - if (!isAllZero - ((const char *)mem1.z, mem1.n)) { - rc = 1; - } else { - rc = mem1.n - pKey2->u.nZero; - } - } else { - int nCmp; - do_bin_cmp: - nCmp = MIN(mem1.n, pKey2->n); - rc = memcmp(mem1.z, pKey2->z, nCmp); - if (rc == 0) - rc = mem1.n - pKey2->n; - } - } else { - rc = 1; - } - break; - } - case MP_ARRAY: - case MP_MAP: { - mem1.z = (char *)aKey1; - mp_next(&aKey1); - mem1.n = aKey1 - (char *)mem1.z; - goto do_blob; - } - case MP_EXT: { - int8_t type; - const char *buf = aKey1; - uint32_t len = mp_decode_extl(&aKey1, &type); - if (type == MP_UUID) { - assert(len == UUID_LEN); - mem1.type = MEM_TYPE_UUID; - aKey1 = buf; - if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL || - mem_cmp_uuid(&mem1, pKey2, &rc) != 0) - rc = 1; - break; - } - aKey1 += len; - mem1.z = (char *)buf; - mem1.n = aKey1 - buf; - goto do_blob; - } - } - *key1 = aKey1; - return rc; -} - int sqlVdbeRecordCompareMsgpack(const void *key1, struct UnpackedRecord *key2) @@ -2721,13 +2587,14 @@ sqlVdbeRecordCompareMsgpack(const void *key1, n = MIN(n, key2->nField); for (i = 0; i != n; i++) { - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); + struct key_part *part = &key2->key_def->parts[i]; + struct Mem *mem = key2->aMem + i; + struct coll *coll = part->coll; + mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll); if (rc != 0) { - if (key2->key_def->parts[i].sort_order != - SORT_ORDER_ASC) { - rc = -rc; - } - return rc; + if (part->sort_order != SORT_ORDER_ASC) + return rc; + return -rc; } } diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index bbb99c4d2..c73536efb 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -704,6 +704,16 @@ int mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, const struct coll *coll); +/** + * Compare MEM and packed to msgpack value using SCALAR rules and return the + * result of comparison. Both values should be scalars. Original MEM is not + * changed. If successful, the second argument will contain the address + * following the specified packed value. + */ +int +mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, + const struct coll *coll); + /** * Convert the given MEM to INTEGER. This function and the function below define * the rules that are used to convert values of all other types to INTEGER. In @@ -981,20 +991,6 @@ int sqlVdbeMemTooBig(Mem *); int sql_vdbemem_finalize(struct Mem *mem, struct func *func); -/** MEM and msgpack functions. */ - -/** - * Perform comparison of two keys: one is packed and one is not. - * - * @param key1 Pointer to pointer to first key. - * @param unpacked Pointer to unpacked tuple. - * @param key2_idx index of key in umpacked record to compare. - * - * @retval +1 if key1 > pUnpacked[iKey2], -1 ptherwise. - */ -int sqlVdbeCompareMsgpack(const char **key1, - struct UnpackedRecord *unpacked, int key2_idx); - /** * Perform comparison of two tuples: unpacked (key1) and packed (key2) * diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua index 6b4a811c3..dd041c0b4 100755 --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(8) +test:plan(9) -- Make sure that function quote() can work with uuid. test:do_execsql_test( @@ -76,6 +76,18 @@ test:do_execsql_test( 2 }) +box.execute([[DELETE FROM t;]]) + +box.execute([[INSERT INTO t VALUES (1, X'00'), (2, ?);]], {uuid1}) + +test:do_execsql_test( + "gh-6164-9", + [[ + SELECT i FROM t ORDER BY s; + ]], { + 1, 2 + }) + box.execute([[DROP TABLE t;]]) test:finish_test()
Thanks for the fixes! See 2 comments below. > diff --git a/src/box/sql.c b/src/box/sql.c > index 790ca7f70..a5afcfabb 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -765,10 +765,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, > } > } > next_fieldno = fieldno + 1; > - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); > + struct key_part *part = &unpacked->key_def->parts[i]; > + struct Mem *mem = unpacked->aMem + i; > + struct coll *coll = part->coll; > + mem_cmp_msgpack(mem, &p, &rc, coll); 1. The same as in the previous patch - you either need to fill rc out parameter even in case of a fail, or check result of mem_cmp_msgpack() to keep the results stable. Not depending on the stack's garbage content. The same in the other places where it is called. > if (rc != 0) { > - if (unpacked->key_def->parts[i].sort_order != > - SORT_ORDER_ASC) > + if (part->sort_order == SORT_ORDER_ASC) > rc = -rc; > goto out; > } > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index da27cd191..4062ff4b3 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -2077,35 +2077,78 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, <...> > + case MP_EXT: { > + int8_t type; > + const char *buf = *b; > + uint32_t len = mp_decode_extl(&buf, &type); > + if (type == MP_UUID) { > + assert(len == UUID_LEN); > + mem.type = MEM_TYPE_UUID; > + if (mp_decode_uuid(b, &mem.u.uuid) == NULL) > + return -1; > + break; > + } > + len += buf - *b; > + mem.type = MEM_TYPE_BIN; > + mem.z = (char *)*b; > + mem.n = len; > + mem.flags = MEM_Ephem; > + *b += len; > + break; 2. I just remembered now, that we have uuid_unpack(). It is better to use when you already decoded the EXT header. Consider this: ==================== @@ -2129,20 +2129,18 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, case MP_EXT: { int8_t type; const char *buf = *b; - uint32_t len = mp_decode_extl(&buf, &type); + uint32_t len = mp_decode_extl(b, &type); if (type == MP_UUID) { - assert(len == UUID_LEN); - mem.type = MEM_TYPE_UUID; - if (mp_decode_uuid(b, &mem.u.uuid) == NULL) + if (uuid_unpack(&b, len, &mem.u.uuid) == NULL) return -1; + mem.type = MEM_TYPE_UUID; break; } - len += buf - *b; mem.type = MEM_TYPE_BIN; - mem.z = (char *)*b; - mem.n = len; + mem.z = (char *)buf; + mem.n = b - buf + len; mem.flags = MEM_Ephem; - *b += len; + b += len; break; } default: ==================== (I didn't test though.)
Thank you for the review! My answers, diff and new patch below. On Mon, Jul 12, 2021 at 11:09:02PM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > See 2 comments below. > > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 790ca7f70..a5afcfabb 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -765,10 +765,12 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, > > } > > } > > next_fieldno = fieldno + 1; > > - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); > > + struct key_part *part = &unpacked->key_def->parts[i]; > > + struct Mem *mem = unpacked->aMem + i; > > + struct coll *coll = part->coll; > > + mem_cmp_msgpack(mem, &p, &rc, coll); > > 1. The same as in the previous patch - you either need to fill > rc out parameter even in case of a fail, or check result of > mem_cmp_msgpack() to keep the results stable. Not depending on > the stack's garbage content. > > The same in the other places where it is called. > Fixed. However, since there is no proper way to throw an error from both functions where mem_cmp_msgpack() was used, I just made rc equal to 0. > > if (rc != 0) { > > - if (unpacked->key_def->parts[i].sort_order != > > - SORT_ORDER_ASC) > > + if (part->sort_order == SORT_ORDER_ASC) > > rc = -rc; > > goto out; > > } > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index da27cd191..4062ff4b3 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -2077,35 +2077,78 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, > > <...> > > > + case MP_EXT: { > > + int8_t type; > > + const char *buf = *b; > > + uint32_t len = mp_decode_extl(&buf, &type); > > + if (type == MP_UUID) { > > + assert(len == UUID_LEN); > > + mem.type = MEM_TYPE_UUID; > > + if (mp_decode_uuid(b, &mem.u.uuid) == NULL) > > + return -1; > > + break; > > + } > > + len += buf - *b; > > + mem.type = MEM_TYPE_BIN; > > + mem.z = (char *)*b; > > + mem.n = len; > > + mem.flags = MEM_Ephem; > > + *b += len; > > + break; > > 2. I just remembered now, that we have uuid_unpack(). It is better > to use when you already decoded the EXT header. Consider this: > > ==================== > @@ -2129,20 +2129,18 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, > case MP_EXT: { > int8_t type; > const char *buf = *b; > - uint32_t len = mp_decode_extl(&buf, &type); > + uint32_t len = mp_decode_extl(b, &type); > if (type == MP_UUID) { > - assert(len == UUID_LEN); > - mem.type = MEM_TYPE_UUID; > - if (mp_decode_uuid(b, &mem.u.uuid) == NULL) > + if (uuid_unpack(&b, len, &mem.u.uuid) == NULL) > return -1; > + mem.type = MEM_TYPE_UUID; > break; > } > - len += buf - *b; > mem.type = MEM_TYPE_BIN; > - mem.z = (char *)*b; > - mem.n = len; > + mem.z = (char *)buf; > + mem.n = b - buf + len; > mem.flags = MEM_Ephem; > - *b += len; > + b += len; > break; > } > default: > ==================== > > (I didn't test though.) Thank you for the suggestion! I applied your diff with some changes. Diff: diff --git a/src/box/sql.c b/src/box/sql.c index a5afcfabb..c1271ee0a 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -768,7 +768,9 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, struct key_part *part = &unpacked->key_def->parts[i]; struct Mem *mem = unpacked->aMem + i; struct coll *coll = part->coll; - mem_cmp_msgpack(mem, &p, &rc, coll); + /* In case of fail make rc equal to 0. */ + if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) + rc = 0; if (rc != 0) { if (part->sort_order == SORT_ORDER_ASC) rc = -rc; diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 4062ff4b3..8e176e418 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -2129,20 +2129,19 @@ mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, case MP_EXT: { int8_t type; const char *buf = *b; - uint32_t len = mp_decode_extl(&buf, &type); + uint32_t len = mp_decode_extl(b, &type); if (type == MP_UUID) { assert(len == UUID_LEN); mem.type = MEM_TYPE_UUID; - if (mp_decode_uuid(b, &mem.u.uuid) == NULL) + if (uuid_unpack(b, len, &mem.u.uuid) == NULL) return -1; break; } - len += buf - *b; + *b += len; mem.type = MEM_TYPE_BIN; - mem.z = (char *)*b; - mem.n = len; + mem.z = (char *)buf; + mem.n = *b - buf; mem.flags = MEM_Ephem; - *b += len; break; } default: @@ -2590,7 +2589,9 @@ sqlVdbeRecordCompareMsgpack(const void *key1, struct key_part *part = &key2->key_def->parts[i]; struct Mem *mem = key2->aMem + i; struct coll *coll = part->coll; - mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll); + /* In case of fail make rc equal to 0. */ + if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) + rc = 0; if (rc != 0) { if (part->sort_order != SORT_ORDER_ASC) return rc; New patch: commit 2f80030b9914256bf931c95b8d76113f31c01655 Author: Mergen Imeev <imeevma@gmail.com> Date: Fri Jul 9 11:59:34 2021 +0300 sql: introduce mem_cmp_msgpack() This patch introduces the mem_cmp_msgpack() function that compares MEM and packed to msgpack value using SCALAR rules. MEM and packed value must be scalars. Prior to this patch, there was a function that used SCALAR rules to compare MEM and packed value, but its design became overly complex as new types appeared. Closes #6164 diff --git a/src/box/sql.c b/src/box/sql.c index 790ca7f70..c1271ee0a 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -765,10 +765,14 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, } } next_fieldno = fieldno + 1; - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); + struct key_part *part = &unpacked->key_def->parts[i]; + struct Mem *mem = unpacked->aMem + i; + struct coll *coll = part->coll; + /* In case of fail make rc equal to 0. */ + if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) + rc = 0; if (rc != 0) { - if (unpacked->key_def->parts[i].sort_order != - SORT_ORDER_ASC) + if (part->sort_order == SORT_ORDER_ASC) rc = -rc; goto out; } diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index da27cd191..8e176e418 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -2077,35 +2077,77 @@ mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, return 0; } -/* - * Both *pMem1 and *pMem2 contain string values. Compare the two values - * using the collation sequence pColl. As usual, return a negative , zero - * or positive value if *pMem1 is less than, equal to or greater than - * *pMem2, respectively. Similar in spirit to "rc = (*pMem1) - (*pMem2);". - * - * Strungs assume to be UTF-8 encoded - */ -static int -vdbeCompareMemString(const Mem * pMem1, const Mem * pMem2, - const struct coll * pColl) -{ - return pColl->cmp(pMem1->z, (size_t)pMem1->n, - pMem2->z, (size_t)pMem2->n, pColl); -} - -/* - * The input pBlob is guaranteed to be a Blob that is not marked - * with MEM_Zero. Return true if it could be a zero-blob. - */ -static int -isAllZero(const char *z, int n) -{ - int i; - for (i = 0; i < n; i++) { - if (z[i]) - return 0; +int +mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, + const struct coll *coll) +{ + struct Mem mem; + switch (mp_typeof(**b)) { + case MP_NIL: + mem.type = MEM_TYPE_NULL; + mp_decode_nil(b); + break; + case MP_BOOL: + mem.type = MEM_TYPE_BOOL; + mem.u.b = mp_decode_bool(b); + break; + case MP_UINT: + mem.type = MEM_TYPE_UINT; + mem.u.u = mp_decode_uint(b); + break; + case MP_INT: + mem.type = MEM_TYPE_INT; + mem.u.i = mp_decode_int(b); + break; + case MP_FLOAT: + mem.type = MEM_TYPE_DOUBLE; + mem.u.r = mp_decode_float(b); + break; + case MP_DOUBLE: + mem.type = MEM_TYPE_DOUBLE; + mem.u.r = mp_decode_double(b); + break; + case MP_STR: + mem.type = MEM_TYPE_STR; + mem.n = mp_decode_strl(b); + mem.z = (char *)*b; + *b += mem.n; + mem.flags = MEM_Ephem; + break; + case MP_BIN: + mem.type = MEM_TYPE_BIN; + mem.n = mp_decode_binl(b); + mem.z = (char *)*b; + *b += mem.n; + mem.flags = MEM_Ephem; + break; + case MP_ARRAY: + case MP_MAP: + mp_next(b); + *result = -1; + return 0; + case MP_EXT: { + int8_t type; + const char *buf = *b; + uint32_t len = mp_decode_extl(b, &type); + if (type == MP_UUID) { + assert(len == UUID_LEN); + mem.type = MEM_TYPE_UUID; + if (uuid_unpack(b, len, &mem.u.uuid) == NULL) + return -1; + break; + } + *b += len; + mem.type = MEM_TYPE_BIN; + mem.z = (char *)buf; + mem.n = *b - buf; + mem.flags = MEM_Ephem; + break; } - return 1; + default: + unreachable(); + } + return mem_cmp_scalar(a, &mem, result, coll); } char * @@ -2534,183 +2576,6 @@ sql_vdbemem_finalize(struct Mem *mem, struct func *func) return ctx.is_aborted ? -1 : 0; } -int -sqlVdbeCompareMsgpack(const char **key1, - struct UnpackedRecord *unpacked, int key2_idx) -{ - const char *aKey1 = *key1; - Mem *pKey2 = unpacked->aMem + key2_idx; - Mem mem1; - int rc = 0; - switch (mp_typeof(*aKey1)) { - default:{ - /* FIXME */ - rc = -1; - break; - } - case MP_NIL:{ - rc = -(pKey2->type != MEM_TYPE_NULL); - mp_decode_nil(&aKey1); - break; - } - case MP_BOOL:{ - mem1.u.b = mp_decode_bool(&aKey1); - if (pKey2->type == MEM_TYPE_BOOL) { - if (mem1.u.b != pKey2->u.b) - rc = mem1.u.b ? 1 : -1; - } else { - rc = pKey2->type == MEM_TYPE_NULL ? 1 : -1; - } - break; - } - case MP_UINT:{ - mem1.u.u = mp_decode_uint(&aKey1); - if (pKey2->type == MEM_TYPE_INT) { - rc = +1; - } else if (pKey2->type == MEM_TYPE_UINT) { - if (mem1.u.u < pKey2->u.u) - rc = -1; - else if (mem1.u.u > pKey2->u.u) - rc = +1; - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - rc = double_compare_uint64(pKey2->u.r, - mem1.u.u, -1); - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_INT:{ - mem1.u.i = mp_decode_int(&aKey1); - if (pKey2->type == MEM_TYPE_UINT) { - rc = -1; - } else if (pKey2->type == MEM_TYPE_INT) { - if (mem1.u.i < pKey2->u.i) { - rc = -1; - } else if (mem1.u.i > pKey2->u.i) { - rc = +1; - } - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - rc = double_compare_nint64(pKey2->u.r, mem1.u.i, - -1); - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_FLOAT:{ - mem1.u.r = mp_decode_float(&aKey1); - goto do_float; - } - case MP_DOUBLE:{ - mem1.u.r = mp_decode_double(&aKey1); - do_float: - if (pKey2->type == MEM_TYPE_INT) { - rc = double_compare_nint64(mem1.u.r, pKey2->u.i, - 1); - } else if (pKey2->type == MEM_TYPE_UINT) { - rc = double_compare_uint64(mem1.u.r, - pKey2->u.u, 1); - } else if (pKey2->type == MEM_TYPE_DOUBLE) { - if (mem1.u.r < pKey2->u.r) { - rc = -1; - } else if (mem1.u.r > pKey2->u.r) { - rc = +1; - } - } else if (pKey2->type == MEM_TYPE_NULL) { - rc = 1; - } else if (pKey2->type == MEM_TYPE_BOOL) { - rc = 1; - } else { - rc = -1; - } - break; - } - case MP_STR:{ - if (pKey2->type == MEM_TYPE_STR) { - struct key_def *key_def = unpacked->key_def; - mem1.n = mp_decode_strl(&aKey1); - mem1.z = (char *)aKey1; - aKey1 += mem1.n; - struct coll *coll = - key_def->parts[key2_idx].coll; - if (coll != NULL) { - mem1.type = MEM_TYPE_STR; - mem1.flags = 0; - rc = vdbeCompareMemString(&mem1, pKey2, - coll); - } else { - goto do_bin_cmp; - } - } else { - rc = pKey2->type == MEM_TYPE_BIN ? -1 : +1; - } - break; - } - case MP_BIN:{ - mem1.n = mp_decode_binl(&aKey1); - mem1.z = (char *)aKey1; - aKey1 += mem1.n; - do_blob: - if (pKey2->type == MEM_TYPE_BIN) { - if (pKey2->flags & MEM_Zero) { - if (!isAllZero - ((const char *)mem1.z, mem1.n)) { - rc = 1; - } else { - rc = mem1.n - pKey2->u.nZero; - } - } else { - int nCmp; - do_bin_cmp: - nCmp = MIN(mem1.n, pKey2->n); - rc = memcmp(mem1.z, pKey2->z, nCmp); - if (rc == 0) - rc = mem1.n - pKey2->n; - } - } else { - rc = 1; - } - break; - } - case MP_ARRAY: - case MP_MAP: { - mem1.z = (char *)aKey1; - mp_next(&aKey1); - mem1.n = aKey1 - (char *)mem1.z; - goto do_blob; - } - case MP_EXT: { - int8_t type; - const char *buf = aKey1; - uint32_t len = mp_decode_extl(&aKey1, &type); - if (type == MP_UUID) { - assert(len == UUID_LEN); - mem1.type = MEM_TYPE_UUID; - aKey1 = buf; - if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL || - mem_cmp_uuid(&mem1, pKey2, &rc) != 0) - rc = 1; - break; - } - aKey1 += len; - mem1.z = (char *)buf; - mem1.n = aKey1 - buf; - goto do_blob; - } - } - *key1 = aKey1; - return rc; -} - int sqlVdbeRecordCompareMsgpack(const void *key1, struct UnpackedRecord *key2) @@ -2721,13 +2586,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1, n = MIN(n, key2->nField); for (i = 0; i != n; i++) { - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); + struct key_part *part = &key2->key_def->parts[i]; + struct Mem *mem = key2->aMem + i; + struct coll *coll = part->coll; + /* In case of fail make rc equal to 0. */ + if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) + rc = 0; if (rc != 0) { - if (key2->key_def->parts[i].sort_order != - SORT_ORDER_ASC) { - rc = -rc; - } - return rc; + if (part->sort_order != SORT_ORDER_ASC) + return rc; + return -rc; } } diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index bbb99c4d2..c73536efb 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -704,6 +704,16 @@ int mem_cmp_scalar(const struct Mem *a, const struct Mem *b, int *result, const struct coll *coll); +/** + * Compare MEM and packed to msgpack value using SCALAR rules and return the + * result of comparison. Both values should be scalars. Original MEM is not + * changed. If successful, the second argument will contain the address + * following the specified packed value. + */ +int +mem_cmp_msgpack(const struct Mem *a, const char **b, int *result, + const struct coll *coll); + /** * Convert the given MEM to INTEGER. This function and the function below define * the rules that are used to convert values of all other types to INTEGER. In @@ -981,20 +991,6 @@ int sqlVdbeMemTooBig(Mem *); int sql_vdbemem_finalize(struct Mem *mem, struct func *func); -/** MEM and msgpack functions. */ - -/** - * Perform comparison of two keys: one is packed and one is not. - * - * @param key1 Pointer to pointer to first key. - * @param unpacked Pointer to unpacked tuple. - * @param key2_idx index of key in umpacked record to compare. - * - * @retval +1 if key1 > pUnpacked[iKey2], -1 ptherwise. - */ -int sqlVdbeCompareMsgpack(const char **key1, - struct UnpackedRecord *unpacked, int key2_idx); - /** * Perform comparison of two tuples: unpacked (key1) and packed (key2) * diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua index 6b4a811c3..dd041c0b4 100755 --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(8) +test:plan(9) -- Make sure that function quote() can work with uuid. test:do_execsql_test( @@ -76,6 +76,18 @@ test:do_execsql_test( 2 }) +box.execute([[DELETE FROM t;]]) + +box.execute([[INSERT INTO t VALUES (1, X'00'), (2, ?);]], {uuid1}) + +test:do_execsql_test( + "gh-6164-9", + [[ + SELECT i FROM t ORDER BY s; + ]], { + 1, 2 + }) + box.execute([[DROP TABLE t;]]) test:finish_test()
Thanks for the fixes! > diff --git a/src/box/sql.c b/src/box/sql.c > index 790ca7f70..c1271ee0a 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -765,10 +765,14 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, > } > } > next_fieldno = fieldno + 1; > - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); > + struct key_part *part = &unpacked->key_def->parts[i]; > + struct Mem *mem = unpacked->aMem + i; > + struct coll *coll = part->coll; > + /* In case of fail make rc equal to 0. */ The comment isn't really necessary I would say. Does not help much. The same in the other place below. > + if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) > + rc = 0; > if (rc != 0) { > - if (unpacked->key_def->parts[i].sort_order != > - SORT_ORDER_ASC) > + if (part->sort_order == SORT_ORDER_ASC) Why did you change != to ==? > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index da27cd191..8e176e418 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -2721,13 +2586,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1, > n = MIN(n, key2->nField); > > for (i = 0; i != n; i++) { > - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); > + struct key_part *part = &key2->key_def->parts[i]; > + struct Mem *mem = key2->aMem + i; > + struct coll *coll = part->coll; > + /* In case of fail make rc equal to 0. */ > + if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) > + rc = 0;
Hi! Thank you for the review! My answers and diff below. On Tue, Jul 13, 2021 at 10:39:04PM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 790ca7f70..c1271ee0a 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -765,10 +765,14 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, > > } > > } > > next_fieldno = fieldno + 1; > > - rc = sqlVdbeCompareMsgpack(&p, unpacked, i); > > + struct key_part *part = &unpacked->key_def->parts[i]; > > + struct Mem *mem = unpacked->aMem + i; > > + struct coll *coll = part->coll; > > + /* In case of fail make rc equal to 0. */ > > The comment isn't really necessary I would say. Does not > help much. The same in the other place below. > Dropped. > > + if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) > > + rc = 0; > > if (rc != 0) { > > - if (unpacked->key_def->parts[i].sort_order != > > - SORT_ORDER_ASC) > > + if (part->sort_order == SORT_ORDER_ASC) > > Why did you change != to ==? > I did this because function sqlVdbeCompareMsgpack() compared packed value as left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed, now MEM is left operand and packed value is right operand. > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index da27cd191..8e176e418 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -2721,13 +2586,16 @@ sqlVdbeRecordCompareMsgpack(const void *key1, > > n = MIN(n, key2->nField); > > > > for (i = 0; i != n; i++) { > > - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); > > + struct key_part *part = &key2->key_def->parts[i]; > > + struct Mem *mem = key2->aMem + i; > > + struct coll *coll = part->coll; > > + /* In case of fail make rc equal to 0. */ > > + if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) > > + rc = 0; Diff: diff --git a/src/box/sql.c b/src/box/sql.c index c1271ee0a..7471c3832 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -768,7 +768,6 @@ tarantoolsqlIdxKeyCompare(struct BtCursor *cursor, struct key_part *part = &unpacked->key_def->parts[i]; struct Mem *mem = unpacked->aMem + i; struct coll *coll = part->coll; - /* In case of fail make rc equal to 0. */ if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) rc = 0; if (rc != 0) { diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 8e176e418..62993fa4f 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -2589,7 +2589,6 @@ sqlVdbeRecordCompareMsgpack(const void *key1, struct key_part *part = &key2->key_def->parts[i]; struct Mem *mem = key2->aMem + i; struct coll *coll = part->coll; - /* In case of fail make rc equal to 0. */ if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) rc = 0; if (rc != 0) {
Hi! Thanks for the fixes! >>> + if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) >>> + rc = 0; >>> if (rc != 0) { >>> - if (unpacked->key_def->parts[i].sort_order != >>> - SORT_ORDER_ASC) >>> + if (part->sort_order == SORT_ORDER_ASC) >> >> Why did you change != to ==? >> > I did this because function sqlVdbeCompareMsgpack() compared packed value as > left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed, > now MEM is left operand and packed value is right operand. Shouldn't you then change the other place too? > @@ -2721,13 +2586,15 @@ sqlVdbeRecordCompareMsgpack(const void *key1, > n = MIN(n, key2->nField); > > for (i = 0; i != n; i++) { > - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); > + struct key_part *part = &key2->key_def->parts[i]; > + struct Mem *mem = key2->aMem + i; > + struct coll *coll = part->coll; > + if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) > + rc = 0; > if (rc != 0) { > - if (key2->key_def->parts[i].sort_order != > - SORT_ORDER_ASC) { > - rc = -rc; > - } > - return rc; > + if (part->sort_order != SORT_ORDER_ASC) > + return rc; > + return -rc; > } Here it was cmp(key1, key), now it is cmp(key2, key1). Shouldn't then the sort_order check become `== SORT_ORDER_ASC`?
Hi! Thank you for the review! My answer below. On 15.07.2021 00:53, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > >>>> + if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0) >>>> + rc = 0; >>>> if (rc != 0) { >>>> - if (unpacked->key_def->parts[i].sort_order != >>>> - SORT_ORDER_ASC) >>>> + if (part->sort_order == SORT_ORDER_ASC) >>> Why did you change != to ==? >>> >> I did this because function sqlVdbeCompareMsgpack() compared packed value as >> left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed, >> now MEM is left operand and packed value is right operand. > Shouldn't you then change the other place too? There I changed returned value instead of operation. > >> @@ -2721,13 +2586,15 @@ sqlVdbeRecordCompareMsgpack(const void *key1, >> n = MIN(n, key2->nField); >> >> for (i = 0; i != n; i++) { >> - rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i); >> + struct key_part *part = &key2->key_def->parts[i]; >> + struct Mem *mem = key2->aMem + i; >> + struct coll *coll = part->coll; >> + if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0) >> + rc = 0; >> if (rc != 0) { >> - if (key2->key_def->parts[i].sort_order != >> - SORT_ORDER_ASC) { >> - rc = -rc; >> - } >> - return rc; >> + if (part->sort_order != SORT_ORDER_ASC) >> + return rc; >> + return -rc; >> } > Here it was cmp(key1, key), now it is cmp(key2, key1). Shouldn't > then the sort_order check become `== SORT_ORDER_ASC`? Although it is true that I have not changed '!=' to '==', now in case '!=' the function returns rc, and in the case of '==' it returns -rc. Previously it was -rc for '!=' and rc for '=='. Is there any other reason to change '!=' to '=='?
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
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
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
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
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
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
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
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
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) : {
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 >