From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 371ED6EC40; Wed, 18 Aug 2021 16:01:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 371ED6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629291672; bh=sepv8gzu7b4475fNMk3UaG8int7r6wdqBsR9V8bUg00=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=cuKYYf87UFtiD27zJOfP4Y/4aNHxJKU8mwLlpH83An6aI8AMVcF5ZicQCiS6NV6ru uAtF9f3AYjlYG73MKgmqwYx376C7XU4emMw0b0fTY6qeYYxUGBMg0tkK0umOPPARN1 EnGpGm+1bnpYFRdJSPi8i1K66ZPgV11BJ2r5Ou7g= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4B55C6EC40 for ; Wed, 18 Aug 2021 16:01:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4B55C6EC40 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mGLBt-0003nz-MR; Wed, 18 Aug 2021 16:01:10 +0300 Date: Wed, 18 Aug 2021 16:01:08 +0300 To: Safin Timur Message-ID: <20210818130108.GA392133@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD910164DC12A5633065676A9727AC27C74182A05F53808504049A9A97A664E7F2F7B7F72C100EFEEA6FFB11A584F936D78DA57C8C7F6338A64 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E5D7EAC6EBA58433EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A6A20D80F0832BC78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A18949FC7B9B7D95908BEB5E29F010AA117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6B1CFA6D474D4A6A4089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5050A8E7512C1707E8F5D81C0DB31B5DC45 X-C1DE0DAB: 0D63561A33F958A58C2AA525A45681DCE0ABC384F044521A24577DAF916D73A9D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA757E10A58996CBD514410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D345B8C2BEC800D102130D7CFCF4F6A4244088A2934F871F70687A7EC18A0067C4FBCE4C43ECBEE8BC81D7E09C32AA3244CEF59B5201F6F1A3FAD92C2C2459E38EDA95CA90A1D8AC565FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28trMbeLtIdid5w== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D9BF592955B78BCDF25D4A05C70F4DE4883D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 2/4] sql: introduce field type decimal X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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(