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 A1EB56EC40; Mon, 16 Aug 2021 22:22:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A1EB56EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629141774; bh=El3Gv3BaXzWLhAYhUSnaotzWHCMlIrcwGJ/buugQ2P8=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=CMBzB8SI67zNs+MSM5QRfXtHBGibw3VQmzn5x981fgMlOeovGuL+Bo5ape2+OoUlU yOr/EFIWffNcT3Xdej73tv9+pzvhDupuwKGvZJBevSGfkMlpyiHmXBLLjU2V73gUTx huEsgevVzNS+lMBpqDd4ikFuppMHLtMiV1PkZsq4= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 E71FC6EC40 for ; Mon, 16 Aug 2021 22:22:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E71FC6EC40 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1mFiCC-0006Zs-NW; Mon, 16 Aug 2021 22:22:53 +0300 To: imeevma@tarantool.org, korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: Message-ID: Date: Mon, 16 Aug 2021 22:22:42 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9BCE6B93DE0C6C3914462CDB1732D383C182A05F5380850407CB56845DA1EC559E6BD60F987335A8BDB8E5D4849C4BD8B88624B17CAE55055 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE718E10EF74DC9F9D0EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063745B6F93C788775E78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D886D0215AF69C8C869CADBA130B59FBD2117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE902A1BE408319B291DBC1C451FC279AAD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3034D30FDF2F620DB9735652A29929C6CC4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637427B078F297B269AEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79D0B18DC6AC13D9A1CAB897378C25451B5 X-C1DE0DAB: 0D63561A33F958A56937DC07FC539670C345470467D23F9ACC7D0D511768AB76D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7567C209D01CC1E34B410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344E332383F80D58BB07D9537107E566566DF3560B339BD42C610A990B9D4A89D0DBF1ABFBC7F0706A1D7E09C32AA3244C530937C95850A2A7854FED41D30AB67FE646F07CC2D4F3D8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojIrFL/N5KnVHmenRtyI0+Cw== X-Mailru-Sender: B5B6A6EBBD94DAD86FFBA9D33F758965C3C720CF712A39F7A78CA9C10AB015BE6E2FE3251B6991485C2808D6142752370A8ED71B308007E3DC85537438B7E1A423D748DE48713E689437F6177E88F7363CDA0F3B3F5B9367 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: Safin Timur via Tarantool-patches Reply-To: Safin Timur Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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