[Tarantool-patches] [PATCH v1 2/4] sql: introduce field type decimal
Safin Timur
tsafin at tarantool.org
Wed Aug 18 19:52:42 MSK 2021
LGTM.
(Though this apparent interdependency between patches is a good
indication that they better to live in a single patch, but I do not insist)
Timur
On 18.08.2021 16:01, Mergen Imeev wrote:
> Hi! Thank ypu fpr the review! My answers and diff below.
>
> On Mon, Aug 16, 2021 at 10:22:42PM +0300, Safin Timur wrote:
>> 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).
>>
> Added.
>
>>> { "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.
>>
> Added in another patch.
>
>>> 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.
>>
>>
> Added in another patch.
>
>>> 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?
>>
>>
> Added in another patch.
>
>>> }
>>> - 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..
>>
> Added in another patch.
>
>>> 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.
>>
> Added, test fixed.
>
>>> 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`.
>>
> True, fixed.
>
>>> + }
>>> +
>>> + 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?
>>
> I see no reason to test both vinyl and memtx.
>
>>> "gh-4077-iproto-execute-no-bind.test.lua": {},
>>> "*": {
>>> "memtx": {"engine": "memtx"},
>>
>> Thanks,
>> Timur
>
>
> Diff:
>
>
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 1c9d12295..3e4200417 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -196,6 +196,7 @@ static Keyword aKeywordTable[] = {
> { "CURRENT_TIMESTAMP", "TK_STANDARD", true },
> { "DATE", "TK_STANDARD", true },
> { "DATETIME", "TK_STANDARD", true },
> + { "DEC", "TK_DECIMAL", true },
> { "DECIMAL", "TK_DECIMAL", true },
> { "DECLARE", "TK_STANDARD", true },
> { "DENSE_RANK", "TK_STANDARD", true },
> diff --git a/test/sql-tap/decimal.c b/test/sql-tap/decimal.c
> index 4d9d1ce19..fd7c3e0c9 100644
> --- a/test/sql-tap/decimal.c
> +++ b/test/sql-tap/decimal.c
> @@ -16,19 +16,19 @@ is_dec(box_function_ctx_t *ctx, const char *args, const char *args_end)
> return box_error_set(__FILE__, __LINE__, ER_PROC_C,
> "invalid argument count");
> }
> - bool is_uuid;
> + bool is_dec;
> if (mp_typeof(*args) == MP_EXT) {
> const char *str = args;
> int8_t type;
> mp_decode_extl(&str, &type);
> - is_uuid = type == MP_DECIMAL;
> + is_dec = type == MP_DECIMAL;
> } else {
> - is_uuid = false;
> + is_dec = false;
> }
>
> char res[BUF_SIZE];
> memset(res, '\0', BUF_SIZE);
> - char *end = mp_encode_bool(res, is_uuid);
> + char *end = mp_encode_bool(res, is_dec);
> box_return_mp(ctx, res, end);
> return 0;
> }
> diff --git a/test/sql-tap/decimal.test.lua b/test/sql-tap/decimal.test.lua
> index dd69ca370..10217a806 100755
> --- a/test/sql-tap/decimal.test.lua
> +++ b/test/sql-tap/decimal.test.lua
> @@ -9,16 +9,26 @@ local dec = require("decimal")
> local dec1 = dec.new("111")
> local dec2 = dec.new("55555")
> local dec3 = dec.new("3333")
> +local dec4 = dec.new("-13")
> +local dec5 = dec.new("0")
> +local dec6 = dec.new("-0")
>
> -- Check that it is possible to create spaces with DECIMAL field.
> test:do_execsql_test(
> "dec-1",
> [[
> - CREATE TABLE t1 (i INT PRIMARY KEY, u DECIMAL);
> + CREATE TABLE t0 (i INT PRIMARY KEY, u DEC);
> + CREATE TABLE t1 (i INT PRIMARY KEY, u DEC);
> CREATE TABLE t2 (u DECIMAL PRIMARY KEY);
> ]], {
> })
>
> +box.space.T0:insert({1, dec1})
> +box.space.T0:insert({2, dec2})
> +box.space.T0:insert({3, dec3})
> +box.space.T0:insert({4, dec4})
> +box.space.T0:insert({5, dec5})
> +box.space.T0:insert({6, dec6})
> box.space.T1:insert({1, dec1})
> box.space.T1:insert({2, dec2})
> box.space.T1:insert({3, dec3})
> @@ -33,9 +43,9 @@ box.space.T2:insert({dec3})
> test:do_execsql_test(
> "dec-2.1.1",
> [[
> - SELECT * FROM t1;
> + SELECT * FROM t0;
> ]], {
> - 1, dec1, 2, dec2, 3, dec3, 4, dec1, 5, dec1, 6, dec2
> + 1, dec1, 2, dec2, 3, dec3, 4, dec4, 5, dec5, 6, dec6
> })
>
> test:do_execsql_test(
> @@ -50,17 +60,17 @@ test:do_execsql_test(
> test:do_execsql_test(
> "dec-2.2.1",
> [[
> - SELECT * FROM t1 ORDER BY u;
> + SELECT * FROM t0 ORDER BY u;
> ]], {
> - 1, dec1, 4, dec1, 5, dec1, 3, dec3, 2, dec2, 6, dec2
> + 4, dec4, 5, dec5, 6, dec6, 1, dec1, 3, dec3, 2, dec2
> })
>
> test:do_execsql_test(
> "dec-2.2.2",
> [[
> - SELECT * FROM t1 ORDER BY u DESC;
> + SELECT * FROM t0 ORDER BY u DESC;
> ]], {
> - 2, dec2, 6, dec2, 3, dec3, 1, dec1, 4, dec1, 5, dec1
> + 2, dec2, 3, dec3, 1, dec1, 5, dec5, 6, dec6, 4, dec4
> })
>
> test:do_execsql_test(
>
More information about the Tarantool-patches
mailing list