[Tarantool-patches] [PATCH v1 2/4] sql: introduce field type decimal
Safin Timur
tsafin at tarantool.org
Mon Aug 16 22:22:42 MSK 2021
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
More information about the Tarantool-patches
mailing list