[Tarantool-patches] [PATCH v1 2/4] sql: introduce field type decimal
Mergen Imeev
imeevma at tarantool.org
Wed Aug 18 16:01:08 MSK 2021
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