Re: [Tarantool-patches] [PATCH v4 18/53] sql: introduce mem_arithmetic()

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

Thanks for the patch!

See 3 comments below.

On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote:
> This patch introduces mem_arithmetic(). Function mem_arithmetic()
> executes arithmetic operations on the first and the second MEMs and
> writes the result to the third MEM.
> 
> Part of #5818
> ---
>  src/box/sql/mem.c  | 224 +++++++++++++++++++++++++++++++++++++--------
>  src/box/sql/mem.h  |   6 +-
>  src/box/sql/vdbe.c | 111 +---------------------
>  3 files changed, 193 insertions(+), 148 deletions(-)
> 
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index f160439c9..6120939d8 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -370,6 +370,192 @@ mem_concat(struct Mem *left, struct Mem *right, struct Mem *result)
>  	return 0;
>  }
>  
> +int
> +mem_arithmetic(const struct Mem *left, const struct Mem *right,
> +	       struct Mem *result, int op)
> +{
> +	sqlVdbeMemSetNull(result);

1. Lets use mem_clear() directly. You delete sqlVdbeMemSetNull() later
anyway.

> +	result->field_type = FIELD_TYPE_NUMBER;
> +	if (((left->flags | right->flags) & MEM_Null) != 0)
> +		return 0;
> +
> +	int64_t il;
> +	bool is_l_neg;
> +	double dl;
> +	uint16_t type_left = 0;

2. Looks surprising when you use 'l' in 3 variables before
and suddenly 'left' here.

Tbh, the old names with A and B looked easier to read. Up to you,
I can live with both.

> +	if ((left->flags & MEM_Real) != 0) {
> +		dl = left->u.r;
> +		type_left = MEM_Real;
> +	} else if ((left->flags & MEM_Int) != 0) {
> +		il = left->u.i;
> +		type_left = MEM_Int;
> +		is_l_neg = true;
> +	} else if ((left->flags & MEM_UInt) != 0) {
> +		il = left->u.i;
> +		type_left = MEM_UInt;
> +		is_l_neg = false;
> +	} else if ((left->flags & (MEM_Str | MEM_Blob)) != 0) {
> +		if (sql_atoi64(left->z, &il, &is_l_neg, left->n) == 0)
> +			type_left = is_l_neg ? MEM_Int : MEM_UInt;
> +		else if (sqlAtoF(left->z, &dl, left->n) != 0)
> +			type_left = MEM_Real;
> +	}
> +
> +	int64_t ir;
> +	bool is_r_neg;
> +	double dr;
> +	uint16_t type_right = 0;
> +	if ((right->flags & MEM_Real) != 0) {
> +		dr = right->u.r;
> +		type_right = MEM_Real;
> +	} else if ((right->flags & MEM_Int) != 0) {
> +		ir = right->u.i;
> +		type_right = MEM_Int;
> +		is_r_neg = true;
> +	} else if ((right->flags & MEM_UInt) != 0) {
> +		ir = right->u.i;
> +		type_right = MEM_UInt;
> +		is_r_neg = false;
> +	} else if ((right->flags & (MEM_Str | MEM_Blob)) != 0) {
> +		if (sql_atoi64(right->z, &ir, &is_r_neg, right->n) == 0)
> +			type_right = is_r_neg ? MEM_Int : MEM_UInt;
> +		else if (sqlAtoF(right->z, &dr, right->n) != 0)
> +			type_right = MEM_Real;
> +	}
> +
> +	if ((type_right & (MEM_Int | MEM_UInt | MEM_Real)) == 0) {
> +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +			 mem_str(right), "numeric");
> +		return -1;
> +	}
> +	if ((type_left & (MEM_Int | MEM_UInt | MEM_Real)) == 0) {
> +		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +			 mem_str(left), "numeric");
> +		return -1;
> +	}
> +	if (((type_left | type_right) & MEM_Real) != 0) {
> +		if (type_left == MEM_Int)
> +			dl = (double)il;
> +		else if (type_left == MEM_UInt)
> +			dl = (double)(uint64_t)il;
> +
> +		if (type_right == MEM_Int)
> +			dr = (double)ir;
> +		else if (type_right == MEM_UInt)
> +			dr = (double)(uint64_t)ir;
> +
> +		double dres;
> +		switch(op) {
> +		case OP_Add:
> +			dres = dl + dr;
> +			break;
> +		case OP_Subtract:
> +			dres = dl - dr;
> +			break;
> +		case OP_Multiply:
> +			dres = dl * dr;
> +			break;
> +		case OP_Divide:
> +			if (dr == 0.) {
> +				diag_set(ClientError, ER_SQL_EXECUTE,
> +					 "division by zero");
> +				return -1;
> +			}
> +			dres = dl / dr;
> +			break;
> +		case OP_Remainder: {
> +			int64_t il = (int64_t)dl;
> +			int64_t ir = (int64_t)dr;
> +			if (ir == 0) {
> +				diag_set(ClientError, ER_SQL_EXECUTE,
> +					 "division by zero");
> +				return -1;
> +			}
> +			if (ir == -1)
> +				ir = 1;
> +			dres = (double)(il % ir);
> +			break;
> +		}
> +		default:
> +			unreachable();
> +		}
> +		if (sqlIsNaN(dres))
> +			return 0;
> +		result->u.r = dres;
> +		result->flags = MEM_Real;
> +		return 0;
> +	}
> +	int64_t ires;
> +	/*
> +	 * TODO: This is wrong. Both these flags should already be set. This
> +	 * assignment done to not change behaviour of the function, which
> +	 * is currently bugged.
> +	 */
> +	is_l_neg = (left->flags & MEM_Int) != 0;
> +	is_r_neg = (right->flags & MEM_Int) != 0;
> +	bool is_res_neg;
> +	switch(op) {
> +	case OP_Add:
> +		if (sql_add_int(il, is_l_neg, ir, is_r_neg, &ires,
> +				&is_res_neg) != 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "integer is overflowed");
> +			return -1;
> +		}
> +		break;
> +	case OP_Subtract:
> +		if (sql_sub_int(il, is_l_neg, ir, is_r_neg, &ires,
> +				&is_res_neg) != 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "integer is overflowed");
> +			return -1;
> +		}
> +		break;
> +	case OP_Multiply:
> +		if (sql_mul_int(il, is_l_neg, ir, is_r_neg, &ires,
> +				&is_res_neg) != 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "integer is overflowed");
> +			return -1;
> +		}
> +		break;
> +	case OP_Divide:
> +		if (ir == 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "division by zero");
> +			return -1;
> +		}
> +		if (sql_div_int(il, is_l_neg, ir, is_r_neg, &ires,
> +				&is_res_neg) != 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "integer is overflowed");
> +			return -1;
> +		}
> +		break;
> +	case OP_Remainder: {
> +		if (ir == 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "division by zero");
> +			return -1;
> +		}
> +		if (ir == -1)
> +			ir = 1;
> +		if (sql_rem_int(il, is_l_neg, ir, is_r_neg, &ires,
> +				&is_res_neg) != 0) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 "integer is overflowed");
> +			return -1;
> +		}
> +		break;
> +	}
> +	default:
> +		unreachable();
> +	}
> +	result->u.i = ires;
> +	result->flags = is_res_neg ? MEM_Int : MEM_UInt;
> +	return 0;

3. The original code looked shorter and easier to read. Why did you
pad it out so much?

Additionally, since you now have a function for doing arith, you
could make one function for each opcode with common parts as a
static functions in mem.c, and call them right from 'case's in
vdbe.c. So instead of one big case,case,case,case: arith which
also has a switch inside, we would have proper case: plus,
case: multiply, etc. Would be faster and would make each
individual function simpler I think.

The names could be mem_arith_plus(), mem_arith_mul(), mem_arith_minus(),
etc.