Re: [Tarantool-patches] [PATCH v1 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik <korablev@tarantool.org>]

On 07 Nov 15:13, imeevma@tarantool.org wrote:
> This patch makes number to be union of UNSIGNED, INTEGER and
> DOUBLE numeric types. The first two types represented by Tarantool
> UNSIGNED and INTEGER types.

Nit: integer's range includes unsigned's one.

> Currently there is not DOUBLE type in Tarantool.

TBO from commit message it's not clear what does you patch change.
Please, elaborate it. What is more, your patch modifies user-visible
behaviour, so I think doc-bot request is required.

> Closes #4233
> Closes #4463

Please, split patch into two ones: each fixes corresponding problem.

> ---
> https://github.com/tarantool/tarantool/issues/4233
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4233-fix-number-field-type-in-sql
> 
>  src/box/sql/vdbe.c                   |  15 +----
>  src/box/sql/vdbeInt.h                |   1 -
>  src/box/sql/vdbemem.c                |  57 ++-----------------
>  test/sql-tap/cast.test.lua           |  32 +++++------
>  test/sql-tap/e_select1.test.lua      |   2 +-
>  test/sql-tap/numcast.test.lua        | 104 ++++++++++++++++++++++++++++++++++-
>  test/sql-tap/select3.test.lua        |   2 +-
>  test/sql-tap/sort.test.lua           |  12 ++--
>  test/sql-tap/tkt-91e2e8ba6f.test.lua |  12 ++--
>  test/sql/integer-overflow.result     |   2 +-
>  test/sql/types.result                |   2 +-
>  11 files changed, 140 insertions(+), 101 deletions(-)

What I really dislike about this patch is the fact that it eliminates
ability to convert value to floating point representation. As far as I
remember you told that you was working on introducing floating point
field type in NoSQL Tarantool. And after making floating point type
available in SQL part of code you deleted should be resurrected.
I suggest firstly introduce floating point type in NoSQL, then make it
available in SQL and finally rename CAST(source TO NUMBER) -> source TO FLOAT

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 03c63d8..2ceee1a 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1586,7 +1586,6 @@ case OP_Subtract:              /* same as TK_MINUS, in1, in2, out3 */
>  case OP_Multiply:              /* same as TK_STAR, in1, in2, out3 */
>  case OP_Divide:                /* same as TK_SLASH, in1, in2, out3 */
>  case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
> -	char bIntint;   /* Started out as two integer operands */
>  	u32 flags;      /* Combined MEM_* flags from both inputs */
>  	u16 type1;      /* Numeric type of left operand */
>  	u16 type2;      /* Numeric type of right operand */
> @@ -1609,7 +1608,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>  		bool is_lhs_neg = pIn1->flags & MEM_Int;
>  		bool is_rhs_neg = pIn2->flags & MEM_Int;
>  		bool is_res_neg;
> -		bIntint = 1;
>  		switch( pOp->opcode) {
>  		case OP_Add: {
>  			if (sql_add_int(iA, is_lhs_neg, iB, is_rhs_neg,
> @@ -1649,7 +1647,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>  		}
>  		mem_set_int(pOut, iB, is_res_neg);
>  	} else {
> -		bIntint = 0;
>  		if (sqlVdbeRealValue(pIn1, &rA) != 0) {
>  			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
>  				 sql_value_to_diag_str(pIn1), "numeric");
> @@ -1660,6 +1657,7 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>  				 sql_value_to_diag_str(pIn2), "numeric");
>  			goto abort_due_to_error;
>  		}
> +		assert(((type1 | type2) & MEM_Real) != 0);
>  		switch( pOp->opcode) {
>  		case OP_Add:         rB += rA;       break;
>  		case OP_Subtract:    rB -= rA;       break;
> @@ -1685,9 +1683,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>  		}
>  		pOut->u.r = rB;
>  		MemSetTypeFlag(pOut, MEM_Real);
> -		if (((type1|type2)&MEM_Real)==0 && !bIntint) {
> -			mem_apply_integer_type(pOut);
> -		}
>  	}

Seems like this change of OP_Remainder is barely related to patch..