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

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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(