Re: [Tarantool-patches] [PATCH v1 3/7] sql: rework OP_Seek* opcodes

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
> index 47a940c56..5d1d7592e 100644
> --- a/src/box/sql/mem.h
> +++ b/src/box/sql/mem.h
> @@ -1028,3 +1028,20 @@ mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var);
>  char *
>  sql_vdbe_mem_encode_tuple(struct Mem *fields, uint32_t field_count,
>  			  uint32_t *tuple_size, struct region *region);
> +
> +static inline bool
> +is_mem_num_min(const struct Mem *mem)
> +{
> +	return (mem->field_type == FIELD_TYPE_INTEGER &&
> +		mem->type == MEM_TYPE_INT && mem->u.i == INT64_MIN) ||
> +	       (mem->field_type == FIELD_TYPE_UNSIGNED &&
> +		mem->type == MEM_TYPE_UINT && mem->u.u == 0);
> +}
> +
> +static inline bool
> +is_mem_num_max(const struct Mem *mem)

1. Could you please name them to mem_is_num_min() and mem_is_num_max()?
Also what if they are DOUBLE? Should't these functions be called then
mem_is_int_min() and mem_is_int_max()?

> +{
> +	return (mem->field_type == FIELD_TYPE_INTEGER ||
> +		mem->field_type == FIELD_TYPE_UNSIGNED) &&
> +	       mem->type == MEM_TYPE_UINT && mem->u.u == 0;
> +}
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 62f58def9..a69402720 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2577,6 +2526,62 @@ case OP_Close: {
>   *
>   * See also: Found, NotFound, SeekGt, SeekGe, SeekLe
>   */
> +case OP_SeekLT: {       /* jump, in3 */
> +	struct VdbeCursor *cur = p->apCsr[pOp->p1];
> +#ifdef SQL_DEBUG
> +	cur->seekOp = pOp->opcode;
> +#endif
> +	cur->nullRow = 0;
> +	cur->uc.pCursor->iter_type = ITER_LT;
> +
> +	uint32_t len = pOp->p4.i;
> +	assert(pOp->p4type == P4_INT32);
> +	assert(len <= cur->key_def->part_count);
> +	struct Mem *mems = &aMem[pOp->p3];
> +	bool is_le = false;
> +	bool is_zero = false;
> +	for (uint32_t i = 0; i < len; ++i) {
> +		enum field_type type = cur->key_def->parts[i].type;
> +		struct Mem *mem = &mems[i];
> +		if (mem_is_field_compatible(mem, type))
> +			continue;
> +		if (!sql_type_is_numeric(type) || !mem_is_num(mem)) {
> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +				 mem_str(mem), field_type_strs[type]);
> +			goto abort_due_to_error;
> +		}
> +		int cmp = mem_cast_implicit_number(mem, type);
> +		is_le = is_le || cmp > 0;
> +		/*
> +		 * If number before cast were less than min possible for given
> +		 * field type, than there is no point to use iterator since we
> +		 * won't find anything.
> +		 */
> +		is_zero = is_zero || (is_mem_num_min(mem) && cmp < 0);

2. Do you really need this optimization? Seems like a very rare case. The
same in the other places.

Also it looks like a lot of code duplication. But I see why, and again I
don't see a way to reuse all this code easily.

> +	}
> +	if (is_zero) {
> +		assert(pOp->p2 > 0);
> +		VdbeBranchTaken(1, 2);
> +		goto jump_to_p2;
> +	}

<...>

> +case OP_SeekLE: {       /* jump, in3 */

<...>

> +	cur->nullRow = 0;
> +	bool is_eq = (cur->uc.pCursor->hints & OPFLAG_SEEKEQ) != 0;
> +	cur->uc.pCursor->iter_type = is_eq ? ITER_REQ : ITER_LE;
> +	assert(!is_eq || pOp[1].opcode == OP_IdxLT);
> +
> +	uint32_t len = pOp->p4.i;
> +	assert(pOp->p4type == P4_INT32);
> +	assert(len <= cur->key_def->part_count);
> +	struct Mem *mems = &aMem[pOp->p3];
> +	bool is_lt = false;
> +	bool is_zero = false;
> +	for (uint32_t i = 0; i < len; ++i) {
> +		enum field_type type = cur->key_def->parts[i].type;
> +		struct Mem *mem = &mems[i];
> +		if (mem_is_field_compatible(mem, type))
> +			continue;
> +		if (!sql_type_is_numeric(type) || !mem_is_num(mem)) {
> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +				 mem_str(mem), field_type_strs[type]);
> +			goto abort_due_to_error;
>  		}
> -		iKey = i;
> -
> -		/* If the P3 value could not be converted into an integer without
> -		 * loss of information, then special processing is required...
> +		int cmp = mem_cast_implicit_number(mem, type);
> +		is_lt = is_lt || cmp < 0;

3. You needed to find values <= mem. The mem during cast was
turned into mem2 < mem, correct? Then you need to find values
<= mem2. Otherwise you skip mem2 itself. Or am I mistaken
somewhere? Similar questions to all the other usages of
mem_cast_implicit_number for seeking.