Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce literals for DECIMAL

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

Thanks for the patch!

See 5 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index ee21c1ede..2b156284c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3286,6 +3286,27 @@ codeReal(Vdbe * v, const char *z, int negateFlag, int iMem)
>  	}
>  }
>  
> +static void
> +expr_code_dec(struct Parse *parser, struct Expr *expr, bool is_neg, int reg)
> +{
> +	const char *str = expr->u.zToken;
> +	assert(str != NULL);
> +	decimal_t dec;
> +	if (is_neg)
> +		str = tt_sprintf("-%s", str);
> +	if (decimal_from_string(&dec, str) == NULL) {
> +		parser->is_aborted = true;
> +		return;
> +	}
> +	char *value = sqlDbMallocRawNN(sql_get(), sizeof(dec));
> +	if (value == NULL) {
> +		parser->is_aborted = true;
> +		return;
> +	}
> +	memcpy(value, &dec, sizeof(dec));
> +	sqlVdbeAddOp4(parser->pVdbe, OP_Decimal, 0, reg, 0, value, P4_DEC);

1. memcpy() is not needed. For the positive case for sure. For the
negative too, but I didn't bench what is faster - tt_sprintf() or
decimal_minus().

====================
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 2b156284c..cce0e5030 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3291,20 +3291,22 @@ expr_code_dec(struct Parse *parser, struct Expr *expr, bool is_neg, int reg)
 {
 	const char *str = expr->u.zToken;
 	assert(str != NULL);
-	decimal_t dec;
-	if (is_neg)
-		str = tt_sprintf("-%s", str);
-	if (decimal_from_string(&dec, str) == NULL) {
-		parser->is_aborted = true;
-		return;
-	}
-	char *value = sqlDbMallocRawNN(sql_get(), sizeof(dec));
-	if (value == NULL) {
-		parser->is_aborted = true;
-		return;
+	decimal_t *value = sqlDbMallocRawNN(sql_get(), sizeof(*value));
+	if (value == NULL)
+		goto error;
+	if (is_neg) {
+		decimal_t dec;
+		if (decimal_from_string(&dec, str) == NULL)
+			goto error;
+		decimal_minus(value, &dec);
+	} else if (decimal_from_string(value, str) == NULL) {
+		goto error;
 	}
-	memcpy(value, &dec, sizeof(dec));
 	sqlVdbeAddOp4(parser->pVdbe, OP_Decimal, 0, reg, 0, value, P4_DEC);
+	return;
+error:
+	sqlDbFree(sql_get(), value);
+	parser->is_aborted = true;
 }
====================

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 44533fb3e..747fb608b 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -804,6 +804,17 @@ case OP_Real: {            /* same as TK_FLOAT, out2 */
>  	break;
>  }
>  
> +/* Opcode: Decimal * P2 * P4 *
> + * Synopsis: r[P2]=P4
> + *
> + * P4 is a pointer to a DECIMAL value. Write that value into register P2.
> + */
> +case OP_Decimal: {            /* same as TK_DECIMAL, out2 */

2. Lets stick to our code style when write comments:
start them with /** out of functions, and not write them
on the same line as code.

> +	pOut = vdbe_prepare_null_out(p, pOp->p2);
> +	mem_set_dec(pOut, pOp->p4.dec);
> +	break;
> +}
> +
>  /* Opcode: String8 * P2 * P4 *
>   * Synopsis: r[P2]='P4'
>   *
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index e40a1a0b3..121b86029 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -97,6 +97,8 @@ struct VdbeOp {
>  		 * Information about ephemeral space field types and key parts.
>  		 */

3. Not related to the patch, but wtf P4_REAL is stored by a
pointer? 'double' is 8 bytes, it fits perfectly into this
union. But instead it is copied onto the heap?! Could you
please file a ticket to fix that and store it by value?

>  		struct sql_space_info *space_info;
> +		/** P4 contains address of decimal. */
> +		decimal_t *dec;
>  	} p4;
> diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
> index 68d2ae585..5f8e885ce 100755
> --- a/test/sql-tap/cast.test.lua
> +++ b/test/sql-tap/cast.test.lua
> @@ -962,7 +962,7 @@ test:do_catchsql_test(
>  test:do_catchsql_test(
>      "cast-6.2.3",
>      [[
> -        SELECT CAST(1.5 AS BOOLEAN);
> +        SELECT CAST(15e-1 AS BOOLEAN);

4. It might be more readable to use e0 ending everywhere.
The numbers otherwise become unreadable.

5. Please, add some tests for so large numbers that only
DECIMAL would fit them. To prove that VDBE really uses
decimal for them.

Also need to ensure typeof() returns correct types depending
on the literal.