[Tarantool-patches] [PATCH v1 1/1] sql: remove implicit cast for COMPARISON
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Mar 30 01:29:22 MSK 2020
Hi! Thanks for the patch!
See 15 comments below.
I don't think I understood all the changed (because I am very out of
context), so I will revisited many of them when you send v2.
On 20/03/2020 13:34, imeevma at tarantool.org wrote:
> This patch removes implicit cast for comparison. Also, it make
> search using an index work the same way as in case of fullscan in
> most cases.
>
> Closes #4230
> Closes #4783
> ---
> https://github.com/tarantool/tarantool/issues/4230
> https://github.com/tarantool/tarantool/issues/4783
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4230-remove-implicit-cast-for-index-search-second
>
> @ChangeLog Remove implicit cast for comparison gh-4230
1. Worth adding more details. What implicit cast? All of them?
Do we need a docbot request for this?
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 1579cc9..cd626bd 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -1304,6 +1304,9 @@ enum trim_side_mask {
> (X) == FIELD_TYPE_UNSIGNED || \
> (X) == FIELD_TYPE_DOUBLE)
>
> +#define sql_mp_type_is_numeric(X) ((X) == MP_INT || (X) == MP_UINT || \
> + (X) == MP_FLOAT || (X) == MP_DOUBLE)
2. Better make it static inline function.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index e8a029a..8c95fd4 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -660,6 +660,216 @@ vdbe_field_ref_fetch_field(struct vdbe_field_ref *field_ref, uint32_t fieldno)
> }
>
> /**
> + * Prepare mem to use for comparison with value with type UNSIGNED
> + * from index.
> + *
> + * @param[out] mem Mem to prepare for comparison.
> + * @param[in][out] op Operation that was before preperation and
> + * operation after.
3. preperation -> preparation. The same in the next functions.
> + * @param is_eq True if operation was EQ.
> + * @param[out] is_none True if nothing will be found.
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +sql_mem_to_unsigned_for_comp(struct Mem *mem, uint32_t *op, int is_eq,
4. Opcode has type u8 in UnpackedRecord. Better make it the
same here. The same in other places.
5. is_eq is a flag, and therefore should be a boolean. The
same in the next functions.
> + bool *is_none)
> +{
> + assert(*is_none == false);> + uint32_t orig_op = *op;
> + enum mp_type mp_type = sql_value_type(mem);
> + assert(mp_type == MP_DOUBLE || mp_type == MP_INT);
> + if (mp_type == MP_INT || mem->u.r < 0) {
> + if (orig_op == OP_SeekGE || orig_op == OP_SeekGT) {
> + mem_set_u64(mem, 0);
> + *op = OP_SeekGE;
> + return 0;
> + }
> + *is_none = true;
> + return 0;
> + }
> + double d = mem->u.r;
> + if (d >= UINT64_MAX) {
> + if (orig_op == OP_SeekLE || orig_op == OP_SeekLT) {
> + mem_set_u64(mem, UINT64_MAX);
> + *op = OP_SeekLE;
> + return 0;
> + }
> + *is_none = true;
> + return 0;
> + }
> + uint64_t u = (uint64_t)d;
> + if (d != u && is_eq) {
> + *is_none = true;
> + return 0;
> + }
> + mem_set_u64(mem, u);
> + if (d == u)
> + return 0;
> + if (orig_op == OP_SeekGE)
> + *op = OP_SeekGT;
> + else if (orig_op == OP_SeekLT)
> + *op = OP_SeekLE;
> + return 0;
> +}
> +
> +/**
> + * Prepare mem to use for comparison with value with type INTEGER
> + * from index.
> + *
> + * @param[out] mem Mem to prepare for comparison.
> + * @param[in][out] op Operation that was before preperation and
> + * operation after.
> + * @param is_eq True if operation was EQ.
> + * @param[out] is_none True if nothing will be found.
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +sql_mem_to_integer_for_comp(struct Mem *mem, uint32_t *op, int is_eq,
> + bool *is_none)
> +{
> + assert(*is_none == false);
> + uint32_t orig_op = *op;
> + assert(sql_value_type(mem) == MP_DOUBLE);
> + double d = mem->u.r;
> + if (d < INT64_MIN) {
> + if (orig_op == OP_SeekGE || orig_op == OP_SeekGT) {
> + mem_set_i64(mem, INT64_MIN);
> + *op = OP_SeekGE;
> + return 0;
> + }
> + *is_none = true;
> + return 0;
> + }
> + if (d >= UINT64_MAX) {
> + if (orig_op == OP_SeekLE || orig_op == OP_SeekLT) {
> + mem_set_u64(mem, UINT64_MAX);
> + *op = OP_SeekLE;
> + return 0;
> + }
> + *is_none = true;
> + return 0;
> + }
> + int64_t i = (int64_t)d;
> + uint64_t u = (uint64_t)d;
> + if (d != i && d != u && is_eq) {
> + *is_none = true;
> + return 0;
> + }
> + if (d > 0)
6. >= 0?
> + mem_set_u64(mem, u);
> + else
> + mem_set_i64(mem, i);
> + if (d == u || d == i)
> + return 0;
> + if (d >= 0) {
> + if (orig_op == OP_SeekGE)
> + *op = OP_SeekGT;
> + else if (orig_op == OP_SeekLT)
> + *op = OP_SeekLE;
> + return 0;
> + }
> + if (orig_op == OP_SeekLE)
> + *op = OP_SeekLT;
> + else if (orig_op == OP_SeekGT)
> + *op = OP_SeekGE;
> + return 0;
> +}
> +
> +/**
> + * Prepare mem to use for comparison with value with type DOUBLE
> + * from index.
> + *
> + * @param[out] mem Mem to prepare for comparison.
> + * @param[in][out] op Operation that was before preperation and
> + * operation after.
> + * @param is_eq True if operation was EQ.
> + * @param[out] is_none True if nothing will be found.
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +sql_mem_to_double_for_comp(struct Mem *mem, uint32_t *op, int is_eq,
> + bool *is_none)
> +{
> + assert(*is_none == false);
> + uint32_t orig_op = *op;
> + enum mp_type mp_type = sql_value_type(mem);
7. Is there a ticket to stop conveting MEM_* to MP_*? MP_* now
contains all MEM_* and even more, right?
> + assert(mp_type == MP_INT || mp_type == MP_UINT);
> + int64_t i = mem->u.i;
> + uint64_t u = mem->u.u;
> + /* 2^53 == 9007199254740992 */
> + if (mp_type == MP_INT && i > -9007199254740992) {
8. Is it possible to replace this with 'i == (int64)(double)i' ?
The same below. Because these double vs int are always tricky.
And I would like to avoid any constant assumptions here. For
example, you excluded 2^53, but 2^53 can be stored (from what I
saw and tested). So it would be more correct to write >= -9007199254740992,
not >. However the 2-cast solution may work too, and does not
use constants.
> + sqlVdbeMemSetDouble(mem, i);
> + return 0;
> + } else if (mp_type == MP_UINT && u < 9007199254740992) {
> + sqlVdbeMemSetDouble(mem, u);
> + return 0;
> + }
> +
> + double d = mp_type == MP_INT ? (double)i : (double)u;
> + if (is_eq) {
> + if (i != d && u != d)
> + *is_none = true;
> + else
> + sqlVdbeMemSetDouble(mem, d);
> + return 0;
> + }
> +
> + sqlVdbeMemSetDouble(mem, d);
> + if (mp_type == MP_INT) {
> + if (orig_op == OP_SeekGT && d > i)
> + *op = OP_SeekGE;
> + else if (orig_op == OP_SeekGE && d < i)
> + *op = OP_SeekGT;
> + else if (orig_op == OP_SeekLT && d < i)
> + *op = OP_SeekLE;
> + else if (orig_op == OP_SeekLE && d > i)
> + *op = OP_SeekLT;
> + return 0;
> + }
> + if (orig_op == OP_SeekGT && d > u)
> + *op = OP_SeekGE;
> + else if (orig_op == OP_SeekGE && d < u)
> + *op = OP_SeekGT;
> + else if (orig_op == OP_SeekLT && d < u)
> + *op = OP_SeekLE;
> + else if (orig_op == OP_SeekLE && d > u)
> + *op = OP_SeekLT;
> + return 0;
> +}
> +
> +/**
> + * Prepare mem to use for comparison with value with numeric type
9. No mem among the arguments.
> + * from index.
> + *
> + * @param[in][out] r Unpacked tuple that should be prepared for
> + * comparison.
> + * @param i field of unpacked tuple that should be prepared.
> + * @param is_eq True if operation was EQ.
> + * @param[out] is_none True if nothing will be found.
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +sql_prepare_rec_for_comp(struct UnpackedRecord *r, int i, int is_eq,
> + bool *is_none)
> +{
> + struct Mem *mem = &r->aMem[i];
> + enum field_type field_type = r->key_def->parts[i].type;
> + uint32_t opcode = r->opcode;
> + if (field_type == FIELD_TYPE_UNSIGNED)
> + sql_mem_to_unsigned_for_comp(mem, &opcode, is_eq, is_none);
> + else if (field_type == FIELD_TYPE_INTEGER)
> + sql_mem_to_integer_for_comp(mem, &opcode, is_eq, is_none);
> + else
> + sql_mem_to_double_for_comp(mem, &opcode, is_eq, is_none);
> + r->opcode = opcode;
> + return 0;
> +}
> @@ -2133,189 +2336,109 @@ case OP_Lt: /* same as TK_LT, jump, in1, in3 */
> case OP_Le: /* same as TK_LE, jump, in1, in3 */
> case OP_Gt: /* same as TK_GT, jump, in1, in3 */
> case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> - int res, res2; /* Result of the comparison of pIn1 against pIn3 */
> - u32 flags1; /* Copy of initial value of pIn1->flags */
> - u32 flags3; /* Copy of initial value of pIn3->flags */
> -
> + /*
> + * Result of comparison: more than 0 if l > r,
> + * 0 if l == r, less than 0 if l < r.
> + */
> + int cmp_res;
> + /* Result of the operation. */
> + bool result;
> pIn1 = &aMem[pOp->p1];
> pIn3 = &aMem[pOp->p3];
> - flags1 = pIn1->flags;
> - flags3 = pIn3->flags;
> - enum field_type ft_p1 = pIn1->field_type;
> - enum field_type ft_p3 = pIn3->field_type;
> - if ((flags1 | flags3)&MEM_Null) {
> - /* One or both operands are NULL */
> - if (pOp->p5 & SQL_NULLEQ) {
> - /* If SQL_NULLEQ is set (which will only happen if the operator is
> - * OP_Eq or OP_Ne) then take the jump or not depending on whether
> - * or not both operands are null.
> - */
> - assert(pOp->opcode==OP_Eq || pOp->opcode==OP_Ne);
> - assert((flags1 & MEM_Cleared)==0);
> - assert((pOp->p5 & SQL_JUMPIFNULL)==0);
> - if ((flags1&flags3&MEM_Null)!=0
> - && (flags3&MEM_Cleared)==0
> - ) {
> - res = 0; /* Operands are equal */
> - } else {
> - res = 1; /* Operands are not equal */
> - }
> - } else {
> - /* SQL_NULLEQ is clear and at least one operand is NULL,
> - * then the result is always NULL.
> - * The jump is taken if the SQL_JUMPIFNULL bit is set.
> +
> + bool has_scalar = (pIn1->field_type == FIELD_TYPE_SCALAR) ||
> + (pIn3->field_type == FIELD_TYPE_SCALAR);
> + enum mp_type ltype = sql_value_type(pIn1);
> + enum mp_type rtype = sql_value_type(pIn3);
> + assert(ltype != MP_MAP && rtype != MP_MAP);
> + assert(ltype != MP_ARRAY && rtype != MP_ARRAY);
> + int mp_classes_comp = mp_type_classes_comp(rtype, ltype);
> +
> + if (ltype == MP_NIL || rtype == MP_NIL) {
> + if ((pOp->p5 & SQL_NULLEQ) == 0) {
> + /*
> + * SQL_NULLEQ is clear and at least one
> + * operand is NULL, then the result is
> + * always NULL. The jump is taken if the
> + * SQL_JUMPIFNULL bit is set.
> */
> if (pOp->p5 & SQL_STOREP2) {
> pOut = vdbe_prepare_null_out(p, pOp->p2);
> - iCompare = 1; /* Operands are not equal */
> + iCompare = 1;
> REGISTER_TRACE(p, pOp->p2, pOut);
> } else {
> VdbeBranchTaken(2,3);
> - if (pOp->p5 & SQL_JUMPIFNULL) {
> + if (pOp->p5 & SQL_JUMPIFNULL)
> goto jump_to_p2;
> - }
10. I know these places violated our style, but lets keep
them. Since they are not stictly related to the patch. We will
have a chance to fix them later.
> }
> break;
> }
> - } else if (((flags1 | flags3) & MEM_Bool) != 0 ||
> - ((flags1 | flags3) & MEM_Blob) != 0) {
> /*
> - * If one of values is of type BOOLEAN or VARBINARY,
> - * then the second one must be of the same type as
> - * well. Otherwise an error is raised.
> + * If SQL_NULLEQ is set (which will only happen if
> + * the operator is OP_Eq or OP_Ne) then take the
> + * jump or not depending on whether or not both
> + * operands are null.
> */
> - int type_arg1 = flags1 & (MEM_Bool | MEM_Blob);
> - int type_arg3 = flags3 & (MEM_Bool | MEM_Blob);
> - if (type_arg1 != type_arg3) {
> - char *inconsistent_type = type_arg1 != 0 ?
> - mem_type_to_str(pIn3) :
> - mem_type_to_str(pIn1);
> - char *expected_type = type_arg1 != 0 ?
> - mem_type_to_str(pIn1) :
> - mem_type_to_str(pIn3);
> - diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> - inconsistent_type, expected_type);
> - goto abort_due_to_error;
> - }
> - res = sqlMemCompare(pIn3, pIn1, NULL);
> + assert(pOp->opcode == OP_Eq || pOp->opcode == OP_Ne);
> + assert((pOp->p5 & SQL_JUMPIFNULL) == 0);
> + cmp_res = !(ltype == MP_NIL && rtype == MP_NIL);
11. The compiler is likely to help us here, but still lets apply '!'.
It will look
ltype != MP_NIL || rtype != MP_NIL
> + } else if (mp_classes_comp == 0) {
> + cmp_res = sqlMemCompare(pIn3, pIn1, pOp->p4.pColl);
> + } else if (has_scalar) {
> + cmp_res = mp_classes_comp;
> } else {
> - enum field_type type = pOp->p5 & FIELD_TYPE_MASK;
> - if (sql_type_is_numeric(type)) {
> - if ((flags1 | flags3)&MEM_Str) {
> - if ((flags1 & MEM_Str) == MEM_Str) {
> - mem_apply_numeric_type(pIn1);
> - testcase( flags3!=pIn3->flags); /* Possible if pIn1==pIn3 */
> - flags3 = pIn3->flags;
> - }
> - if ((flags3 & MEM_Str) == MEM_Str) {
> - if (mem_apply_numeric_type(pIn3) != 0) {
> - diag_set(ClientError,
> - ER_SQL_TYPE_MISMATCH,
> - sql_value_to_diag_str(pIn3),
> - "numeric");
> - goto abort_due_to_error;
> - }
> + diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> + mem_type_to_str(pIn1), mem_type_to_str(pIn3));
> + goto abort_due_to_error;
> + }
>
> - }
> - }
> - /* Handle the common case of integer comparison here, as an
> - * optimization, to avoid a call to sqlMemCompare()
> - */
> - if ((pIn1->flags & pIn3->flags & (MEM_Int | MEM_UInt)) != 0) {
> - if ((pIn1->flags & pIn3->flags & MEM_Int) != 0) {
> - if (pIn3->u.i > pIn1->u.i)
> - res = +1;
> - else if (pIn3->u.i < pIn1->u.i)
> - res = -1;
> - else
> - res = 0;
> - goto compare_op;
> - }
> - if ((pIn1->flags & pIn3->flags & MEM_UInt) != 0) {
> - if (pIn3->u.u > pIn1->u.u)
> - res = +1;
> - else if (pIn3->u.u < pIn1->u.u)
> - res = -1;
> - else
> - res = 0;
> - goto compare_op;
> - }
> - if ((pIn1->flags & MEM_UInt) != 0 &&
> - (pIn3->flags & MEM_Int) != 0) {
> - res = -1;
> - goto compare_op;
> - }
> - res = 1;
> - goto compare_op;
> - }
> - } else if (type == FIELD_TYPE_STRING) {
> - if ((flags1 & MEM_Str) == 0 &&
> - (flags1 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> - testcase( pIn1->flags & MEM_Int);
> - testcase( pIn1->flags & MEM_Real);
> - sqlVdbeMemStringify(pIn1);
> - testcase( (flags1&MEM_Dyn) != (pIn1->flags&MEM_Dyn));
> - flags1 = (pIn1->flags & ~MEM_TypeMask) | (flags1 & MEM_TypeMask);
> - assert(pIn1!=pIn3);
> - }
> - if ((flags3 & MEM_Str) == 0 &&
> - (flags3 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> - testcase( pIn3->flags & MEM_Int);
> - testcase( pIn3->flags & MEM_Real);
> - sqlVdbeMemStringify(pIn3);
> - testcase( (flags3&MEM_Dyn) != (pIn3->flags&MEM_Dyn));
> - flags3 = (pIn3->flags & ~MEM_TypeMask) | (flags3 & MEM_TypeMask);
> - }
> - }
> - assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl==0);
> - res = sqlMemCompare(pIn3, pIn1, pOp->p4.pColl);
> - }
> - compare_op:
> - switch( pOp->opcode) {
> - case OP_Eq: res2 = res==0; break;
> - case OP_Ne: res2 = res; break;
> - case OP_Lt: res2 = res<0; break;
> - case OP_Le: res2 = res<=0; break;
> - case OP_Gt: res2 = res>0; break;
> - default: res2 = res>=0; break;
> - }
> -
> - /* Undo any changes made by mem_apply_type() to the input registers. */
> - assert((pIn1->flags & MEM_Dyn) == (flags1 & MEM_Dyn));
> - pIn1->flags = flags1;
> - pIn1->field_type = ft_p1;
> - assert((pIn3->flags & MEM_Dyn) == (flags3 & MEM_Dyn));
> - pIn3->flags = flags3;
> - pIn3->field_type = ft_p3;
> + switch (pOp->opcode) {
> + case OP_Eq:
> + result = cmp_res == 0;
> + break;
> + case OP_Ne:
> + result = cmp_res != 0;
> + break;
> + case OP_Lt:
> + result = cmp_res < 0;
> + break;
> + case OP_Le:
> + result = cmp_res <= 0;
> + break;
> + case OP_Gt:
> + result = cmp_res > 0;
> + break;
> + case OP_Ge:
> + result = cmp_res >= 0;
> + break;
> + default:
> + unreachable();
> + }
>
> if (pOp->p5 & SQL_STOREP2) {
> - iCompare = res;
> - res2 = res2!=0; /* For this path res2 must be exactly 0 or 1 */
> - if ((pOp->p5 & SQL_KEEPNULL)!=0) {
> - /* The KEEPNULL flag prevents OP_Eq from overwriting a NULL with true
> - * and prevents OP_Ne from overwriting NULL with false. This flag
> - * is only used in contexts where either:
> - * (1) op==OP_Eq && (r[P2]==NULL || r[P2]==0)
> - * (2) op==OP_Ne && (r[P2]==NULL || r[P2]==1)
> - * Therefore it is not necessary to check the content of r[P2] for
> - * NULL.
> - */
> - assert(pOp->opcode==OP_Ne || pOp->opcode==OP_Eq);
> - assert(res2==0 || res2==1);
> - testcase( res2==0 && pOp->opcode==OP_Eq);
> - testcase( res2==1 && pOp->opcode==OP_Eq);
> - testcase( res2==0 && pOp->opcode==OP_Ne);
> - testcase( res2==1 && pOp->opcode==OP_Ne);
> - if ((pOp->opcode==OP_Eq)==res2) break;
> + iCompare = cmp_res;
> + /*
> + * The KEEPNULL flag prevents OP_Eq from
> + * overwriting a NULL with true and prevents OP_Ne
> + * from overwriting NULL with false. This flag
> + * is only used in contexts where either:
> + * (1) op==OP_Eq && (r[P2]==NULL || r[P2]==0)
> + * (2) op==OP_Ne && (r[P2]==NULL || r[P2]==1)
> + * Therefore it is not necessary to check the
> + * content of r[P2] for NULL.
> + */
> + if ((pOp->p5 & SQL_KEEPNULL) != 0) {
12. This condition and the comment clearly could be kept as is, since
only formatting is changed. Could you please preserve the old lines?
For the sake of git blame. Also check other places where it is possible
to keep the old lines for free (without necessity to make new code
worse).
> + assert(pOp->opcode == OP_Ne || pOp->opcode == OP_Eq);
> + if ((pOp->opcode == OP_Eq) == result)
> + break;
> }
> pOut = vdbe_prepare_null_out(p, pOp->p2);
> - mem_set_bool(pOut, res2);
> + mem_set_bool(pOut, result);
> REGISTER_TRACE(p, pOp->p2, pOut);
> } else {
> - VdbeBranchTaken(res!=0, (pOp->p5 & SQL_NULLEQ)?2:3);
> - if (res2) {
> + VdbeBranchTaken(cmp_res != 0, (pOp->p5 & SQL_NULLEQ) ? 2 : 3);
> + if (result)
> goto jump_to_p2;
> - }
> }
> break;
> }> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index 3f8a0ce..6cbf555 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -104,6 +104,12 @@ mp_classof(enum mp_type type)
> return mp_classes[type];
> }
>
> +int
> +mp_type_classes_comp(enum mp_type ltype, enum mp_type rtype)
> +{
> + return mp_classof(ltype) - mp_classof(rtype);
> +}
13. Probably better keep it in one single usage place. Since it adds
unnecessary dependency on msgpuck.h for tuple_compare.h.
> +
> static enum mp_class
> mp_extension_class(const char *data)
> {
> diff --git a/test/sql-tap/tkt3493.test.lua b/test/sql-tap/tkt3493.test.lua
> index 7ceec47..e5d7b2f 100755
> --- a/test/sql-tap/tkt3493.test.lua
> +++ b/test/sql-tap/tkt3493.test.lua
> @@ -45,7 +45,7 @@ test:do_execsql_test(
> [[
> SELECT
> CASE
> - WHEN B.val = 1 THEN 'XYZ'
> + WHEN B.val = '1' THEN 'XYZ'
14. Leading whitespace.
> ELSE A.val
> END AS Col1
> FROM B
15. I propose to add some tests specific to the tickets we fix.
Probably remove some existing tests which test the same. With more
edge cases. gcov should help to check that all conditions in this
patch are covered, especially about preparations of Mem to be used
in a comparison.
More information about the Tarantool-patches
mailing list