Re: [Tarantool-patches] [PATCH v1 2/4] sql: introduce field type decimal

[Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Please see below several notes...

On 16.08.2021 18:57, Mergen Imeev via Tarantool-patches wrote:
> This patch introduces a decimal field type. However, implicit and
> explicit casts and arithmetic operations for this type will be presented
> in next few patches. Literals also will be introduced later.
> 
> Part of #4415
> ---
>   extra/mkkeywordhash.c                         |   2 +-
>   src/box/sql/expr.c                            |   3 +
>   src/box/sql/func.c                            |   4 +
>   src/box/sql/mem.c                             | 173 +++++--
>   src/box/sql/mem.h                             |  18 +-
>   src/box/sql/parse.y                           |   1 +
>   src/box/sql/sqlInt.h                          |   1 +
>   test/sql-tap/CMakeLists.txt                   |   1 +
>   test/sql-tap/decimal.c                        |  48 ++
>   test/sql-tap/decimal.test.lua                 | 441 ++++++++++++++++++
>   test/sql-tap/engine.cfg                       |   3 +
>   .../gh-5913-segfault-on-select-uuid.test.lua  |  83 ----
>   .../sql-tap/gh-6024-funcs-return-bin.test.lua |   8 +-
>   13 files changed, 661 insertions(+), 125 deletions(-)
>   create mode 100644 test/sql-tap/decimal.c
>   create mode 100755 test/sql-tap/decimal.test.lua
>   delete mode 100755 test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua
> 
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 0d998506c..1c9d12295 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -196,7 +196,7 @@ static Keyword aKeywordTable[] = {
>     { "CURRENT_TIMESTAMP",      "TK_STANDARD",    true  },
>     { "DATE",                   "TK_STANDARD",    true  },
>     { "DATETIME",               "TK_STANDARD",    true  },
> -  { "DECIMAL",                "TK_STANDARD",    true  },
> +  { "DECIMAL",                "TK_DECIMAL",     true  },

DEC is standard alias to DECIMAL. We should support that (similarly to 
INTEGER vs INT).

>     { "DECLARE",                "TK_STANDARD",    true  },
>     { "DENSE_RANK",             "TK_STANDARD",    true  },
>     { "DESCRIBE",               "TK_STANDARD",    true  },
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index c67a7091c..275dbc5ba 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c

...

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 066940fac..016f0e80b 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -1191,6 +1208,10 @@ mem_cast_explicit(struct Mem *mem, enum field_type
> type)
>   		return -1;
>   	case FIELD_TYPE_NUMBER:
>   		return mem_to_number(mem);
> +	case FIELD_TYPE_DECIMAL:
> +		if (mem->type == MEM_TYPE_DEC)
> +			return 0;
> +		return -1;

So CAST(expr as DECIMAL) will only work for DECIMALs? It should be 
rather behaving similar to other numeric types.

>   	case FIELD_TYPE_UUID:
>   		if (mem->type == MEM_TYPE_UUID) {
>   			mem->flags = 0;
> @@ -1274,6 +1295,10 @@ mem_cast_implicit(struct Mem *mem, enum field_type
> type)
>   			return -1;
>   		mem->flags = MEM_Number;
>   		return 0;
> +	case FIELD_TYPE_DECIMAL:
> +		if (mem->type == MEM_TYPE_DEC)
> +			return 0;
> +		return -1;

Same question as above - implicit conversions to decimal should be 
numeric-like.


>   	case FIELD_TYPE_MAP:
>   		if (mem->type == MEM_TYPE_MAP)
>   			return 0;
> @@ -1595,12 +1620,12 @@ mem_concat(struct Mem *a, struct Mem *b, struct
> Mem *result)
>   static inline int
>   check_types_numeric_arithmetic(const struct Mem *a, const struct Mem *b)
>   {
> -	if (!mem_is_num(a) || mem_is_metatype(a)) {
> +	if (!mem_is_num(a) || mem_is_metatype(a) || a->type ==
> MEM_TYPE_DEC) {
>   		diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(a),
>   			 "integer, unsigned or double");
>   		return -1;

I don't understant - we would raise if not a numeric (and decimal is 
part of numeric) or decimal specifically? So you do not want arithmetic 
types with decimals?


>   	}
> -	if (!mem_is_num(b) || mem_is_metatype(b)) {
> +	if (!mem_is_num(b) || mem_is_metatype(b) || b->type ==
> MEM_TYPE_DEC) {

The same confusion as above..

>   		diag_set(ClientError, ER_SQL_TYPE_MISMATCH, mem_str(b),
>   			 "integer, unsigned or double");
>   		return -1;
> @@ -2926,26 +3040,10 @@ port_lua_get_vdbemem(struct port *base, uint32_t
> *size)
>   		case MP_EXT: {
>   			assert(field.ext_type == MP_UUID ||
>   			       field.ext_type == MP_DECIMAL);
> -			char *buf;
> -			uint32_t size;
> -			uint32_t svp = region_used(&fiber()->gc);
> -			if (field.ext_type == MP_UUID) {
> +			if (field.ext_type == MP_UUID)
>   				mem_set_uuid(&val[i], field.uuidval);
> -				break;
> -			} else {
> -				size = mp_sizeof_decimal(field.decval);
> -				buf = region_alloc(&fiber()->gc, size);
> -				if (buf == NULL) {
> -					diag_set(OutOfMemory, size,
> -						 "region_alloc", "buf");
> -					goto error;
> -				}
> -				mp_encode_decimal(buf, field.decval);
> -			}
> -			int rc = mem_copy_bin(&val[i], buf, size);
> -			region_truncate(&fiber()->gc, svp);
> -			if (rc != 0)
> -				goto error;
> +			else
> +				mem_set_dec(&val[i], field.decval);

Nice! Now it's much compacter and is more readable than before!

> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index bd041e862..436c98cd9 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1863,6 +1863,7 @@ number_typedef(A) ::= NUMBER . { A.type =
> FIELD_TYPE_NUMBER; }
>   number_typedef(A) ::= DOUBLE . { A.type = FIELD_TYPE_DOUBLE; }
>   number_typedef(A) ::= INT|INTEGER_KW . { A.type = FIELD_TYPE_INTEGER; }
>   number_typedef(A) ::= UNSIGNED . { A.type = FIELD_TYPE_UNSIGNED; }
> +number_typedef(A) ::= DECIMAL . { A.type = FIELD_TYPE_DECIMAL; }

Here please add alias to DEC, as it was done with INT.

> diff --git a/test/sql-tap/CMakeLists.txt b/test/sql-tap/CMakeLists.txt
> index bd2b9f33f..87f23b2f7 100644
> --- a/test/sql-tap/CMakeLists.txt
> +++ b/test/sql-tap/CMakeLists.txt
> @@ -2,3 +2,4 @@ include_directories(${MSGPUCK_INCLUDE_DIRS})
>   build_module(gh-5938-wrong-string-length gh-5938-wrong-string-length.c)
>   build_module(gh-6024-funcs-return-bin gh-6024-funcs-return-bin.c)
>   build_module(sql_uuid sql_uuid.c)
> +build_module(decimal decimal.c)
> diff --git a/test/sql-tap/decimal.c b/test/sql-tap/decimal.c
> new file mode 100644
> index 000000000..4d9d1ce19
> --- /dev/null
> +++ b/test/sql-tap/decimal.c
> @@ -0,0 +1,48 @@
> +#include "msgpuck.h"
> +#include "module.h"
> +#include "mp_decimal.h"
> +#include "mp_extension_types.h"
> +
> +enum {
> +	BUF_SIZE = 512,
> +};
> +
> +int
> +is_dec(box_function_ctx_t *ctx, const char *args, const char *args_end)
> +{
> +	(void)args_end;
> +	uint32_t arg_count = mp_decode_array(&args);
> +	if (arg_count != 1) {
> +		return box_error_set(__FILE__, __LINE__, ER_PROC_C,
> +				     "invalid argument count");
> +	}
> +	bool is_uuid;
> +	if (mp_typeof(*args) == MP_EXT) {
> +		const char *str = args;
> +		int8_t type;
> +		mp_decode_extl(&str, &type);
> +		is_uuid = type == MP_DECIMAL;
> +	} else {
> +		is_uuid = false;

Here we see remnants from copy-paste from uuid related code, I assume 
you meant that variable should be named `is_decimal`.

> +	}
> +
> +	char res[BUF_SIZE];
> +	memset(res, '\0', BUF_SIZE);
> +	char *end = mp_encode_bool(res, is_uuid);
> +	box_return_mp(ctx, res, end);
> +	return 0;
> +}
> +
> +int
> diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
> index 820c72b00..511d0a716 100644
> --- a/test/sql-tap/engine.cfg
> +++ b/test/sql-tap/engine.cfg
> @@ -26,6 +26,9 @@
>       "metatypes.test.lua": {
>           "memtx": {"engine": "memtx"}
>       },
> +    "decimal.test.lua": {
> +        "memtx": {"engine": "memtx"}
> +    },

BTW, why this exception for the test?

>       "gh-4077-iproto-execute-no-bind.test.lua": {},
>       "*": {
>           "memtx": {"engine": "memtx"},

Thanks,
Timur