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 C3D026EC55; Sat, 9 Oct 2021 01:32:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C3D026EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1633732361; bh=sgf7RCUn/UMx2VjB3QV8x1/M4ou6c4RrUhHIJNgwmR4=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=VWSr7GklxAA1qG6/DRtmTmOCIB4aYQ6742A4+6OJImc+qnFAOA4ivx5b4B3NkXuem rhPwF+G1WiqXgT19QyydvGxxjc7tmPJUU05T8pfqSYnspfX1hgJJdkh/wyBLvgDOOs jd5lDWvOZ2qHsoVWIG1t9cv7CmwdBt+q5VwNcKQw= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 5E8A86EC55 for ; Sat, 9 Oct 2021 01:32:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5E8A86EC55 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1mYyPu-0001fp-JN; Sat, 09 Oct 2021 01:32:39 +0300 Message-ID: <6d07c477-7d5c-1031-33e3-5d09b978b8f4@tarantool.org> Date: Sat, 9 Oct 2021 00:32:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.1.2 Content-Language: en-US To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <7ac4a555ec1237159e423fec7fd6d4df6ac65d3b.1633354455.git.imeevma@gmail.com> In-Reply-To: <7ac4a555ec1237159e423fec7fd6d4df6ac65d3b.1633354455.git.imeevma@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9A6D4E3B1981C4C7D1676026DFCB81A25119F3452106D1766182A05F538085040A805C2B62F3ECABBB6CD095B58E2E3098D9352675B496FFC859B1E80A8A8B8F1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F4E79F226F99D4DDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F08BD7C3AB07DC7F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D875AFF483C1F25D63DEAC406541A521DC117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6B1CFA6D474D4A6A4089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A54287745AF27B39E8ED0994155213BCBB10766DD7BA261072D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA759D2A03B9C34326B3410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34ECB3E21D3CD9CB4FF53D737DCEBE82062A0D5BBAED9B345EF5AA2B01BE2D37A5935004FD1013906B1D7E09C32AA3244CFD5483B2C066D5861C2ABED9170FA62869B6CAE0477E908D729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMZ06aokA6bo1aA23TaAjfA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DFE68174C9A4132B6D6E63BE5E7FCE30D3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce literals for 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for the patch! See 5 comments below. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index ee21c1ede..2b156284c 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -3286,6 +3286,27 @@ codeReal(Vdbe * v, const char *z, int negateFlag, int iMem) > } > } > > +static void > +expr_code_dec(struct Parse *parser, struct Expr *expr, bool is_neg, int reg) > +{ > + const char *str = expr->u.zToken; > + assert(str != NULL); > + decimal_t dec; > + if (is_neg) > + str = tt_sprintf("-%s", str); > + if (decimal_from_string(&dec, str) == NULL) { > + parser->is_aborted = true; > + return; > + } > + char *value = sqlDbMallocRawNN(sql_get(), sizeof(dec)); > + if (value == NULL) { > + parser->is_aborted = true; > + return; > + } > + memcpy(value, &dec, sizeof(dec)); > + sqlVdbeAddOp4(parser->pVdbe, OP_Decimal, 0, reg, 0, value, P4_DEC); 1. memcpy() is not needed. For the positive case for sure. For the negative too, but I didn't bench what is faster - tt_sprintf() or decimal_minus(). ==================== diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 2b156284c..cce0e5030 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3291,20 +3291,22 @@ expr_code_dec(struct Parse *parser, struct Expr *expr, bool is_neg, int reg) { const char *str = expr->u.zToken; assert(str != NULL); - decimal_t dec; - if (is_neg) - str = tt_sprintf("-%s", str); - if (decimal_from_string(&dec, str) == NULL) { - parser->is_aborted = true; - return; - } - char *value = sqlDbMallocRawNN(sql_get(), sizeof(dec)); - if (value == NULL) { - parser->is_aborted = true; - return; + decimal_t *value = sqlDbMallocRawNN(sql_get(), sizeof(*value)); + if (value == NULL) + goto error; + if (is_neg) { + decimal_t dec; + if (decimal_from_string(&dec, str) == NULL) + goto error; + decimal_minus(value, &dec); + } else if (decimal_from_string(value, str) == NULL) { + goto error; } - memcpy(value, &dec, sizeof(dec)); sqlVdbeAddOp4(parser->pVdbe, OP_Decimal, 0, reg, 0, value, P4_DEC); + return; +error: + sqlDbFree(sql_get(), value); + parser->is_aborted = true; } ==================== > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 44533fb3e..747fb608b 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -804,6 +804,17 @@ case OP_Real: { /* same as TK_FLOAT, out2 */ > break; > } > > +/* Opcode: Decimal * P2 * P4 * > + * Synopsis: r[P2]=P4 > + * > + * P4 is a pointer to a DECIMAL value. Write that value into register P2. > + */ > +case OP_Decimal: { /* same as TK_DECIMAL, out2 */ 2. Lets stick to our code style when write comments: start them with /** out of functions, and not write them on the same line as code. > + pOut = vdbe_prepare_null_out(p, pOp->p2); > + mem_set_dec(pOut, pOp->p4.dec); > + break; > +} > + > /* Opcode: String8 * P2 * P4 * > * Synopsis: r[P2]='P4' > * > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h > index e40a1a0b3..121b86029 100644 > --- a/src/box/sql/vdbe.h > +++ b/src/box/sql/vdbe.h > @@ -97,6 +97,8 @@ struct VdbeOp { > * Information about ephemeral space field types and key parts. > */ 3. Not related to the patch, but wtf P4_REAL is stored by a pointer? 'double' is 8 bytes, it fits perfectly into this union. But instead it is copied onto the heap?! Could you please file a ticket to fix that and store it by value? > struct sql_space_info *space_info; > + /** P4 contains address of decimal. */ > + decimal_t *dec; > } p4; > diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua > index 68d2ae585..5f8e885ce 100755 > --- a/test/sql-tap/cast.test.lua > +++ b/test/sql-tap/cast.test.lua > @@ -962,7 +962,7 @@ test:do_catchsql_test( > test:do_catchsql_test( > "cast-6.2.3", > [[ > - SELECT CAST(1.5 AS BOOLEAN); > + SELECT CAST(15e-1 AS BOOLEAN); 4. It might be more readable to use e0 ending everywhere. The numbers otherwise become unreadable. 5. Please, add some tests for so large numbers that only DECIMAL would fit them. To prove that VDBE really uses decimal for them. Also need to ensure typeof() returns correct types depending on the literal.