From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2482E2DA4C for ; Tue, 11 Jun 2019 17:11:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 06M5Hbj9yPze for ; Tue, 11 Jun 2019 17:11:29 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 64CCF2D9DA for ; Tue, 11 Jun 2019 17:11:28 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 4/6] sql: make built-in functions operate on unsigned values References: <4acf9fab112e7ca119f1b2bf5efea53b3831ea4f.1559919361.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4cbc3612-b655-8b2b-d879-6b1445c9535b@tarantool.org> Date: Tue, 11 Jun 2019 23:11:37 +0200 MIME-Version: 1.0 In-Reply-To: <4acf9fab112e7ca119f1b2bf5efea53b3831ea4f.1559919361.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Nikita Pettik Thanks for the patch! See 7 comments below, review fixes at the end of the email, and on the branch in a separate commit. On 07/06/2019 18:37, Nikita Pettik wrote: > As a part of introduction unsigned type in SQL, let's patch all built-in > function to make them accept and operate on unsigned value, i.e. values > which come with MEM_UInt VDBE memory type. > > Part of #3810 > Part of #4015 > --- > src/box/sql/func.c | 70 ++++++++++++++++++------------------------- > src/box/sql/main.c | 3 +- > src/box/sql/sqlInt.h | 9 ++++-- > src/box/sql/vdbeInt.h | 2 +- > src/box/sql/vdbeapi.c | 22 ++++++++++---- > test/sql-tap/cast.test.lua | 8 ++--- > test/sql-tap/check.test.lua | 2 +- > test/sql-tap/cse.test.lua | 6 ++-- > test/sql-tap/default.test.lua | 2 +- > test/sql-tap/fkey2.test.lua | 4 +-- > test/sql-tap/func.test.lua | 18 ++++++++--- > test/sql-tap/table.test.lua | 2 +- > test/sql-tap/tkt3493.test.lua | 4 +-- > 13 files changed, 82 insertions(+), 70 deletions(-) > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 457c9b92b..365f8f9d2 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -645,21 +638,13 @@ ICU_CASE_CONVERT(Upper); > static void > randomFunc(sql_context * context, int NotUsed, sql_value ** NotUsed2) > { > - sql_int64 r; > + int64_t r; > UNUSED_PARAMETER2(NotUsed, NotUsed2); > sql_randomness(sizeof(r), &r); > - if (r < 0) { > - /* We need to prevent a random number of 0x8000000000000000 > - * (or -9223372036854775808) since when you do abs() of that > - * number of you get the same value back again. To do this > - * in a way that is testable, mask the sign bit off of negative > - * values, resulting in a positive value. Then take the > - * 2s complement of that positive value. The end result can > - * therefore be no less than -9223372036854775807. > - */ > - r = -(r & LARGEST_INT64); > - } > - sql_result_int64(context, r); > + if (r < 0) > + sql_result_int(context, r); > + else > + sql_result_uint(context, r); 1. To avoid such mess I propose you the same as for mem_set_int - make several functions: /* Return an int64 value, sign is detected inside. */ sql_result_i64(int64_t value); /* Return a uint64 value. */ sql_result_u64(uint64_t value); /* Return an integer value with explicit sign. */ sql_result_int(int64_t value, bool is_negative); > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index c0e2ab699..4697e3003 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -455,6 +455,9 @@ sql_column_subtype(struct sql_stmt *stmt, int i); > sql_int64 > sql_value_int64(sql_value *); > > +uint64_t > +sql_value_uint64(sql_value *val); > + > const unsigned char * > sql_value_text(sql_value *); > > @@ -496,13 +499,13 @@ void > sql_result_error_code(sql_context *, int); > > void > -sql_result_int(sql_context *, int); > +sql_result_int(sql_context *, int64_t); > > void > -sql_result_bool(struct sql_context *ctx, bool value); > +sql_result_uint(sql_context *ctx, int64_t u_val); 2. Why result_uint takes int instead of uint? 3. Please, move 'result_uint' above 'result_int' to avoid messing diff with 'result_bool'. > > void > -sql_result_int64(sql_context *, sql_int64); > +sql_result_bool(struct sql_context *ctx, bool value); > > void > sql_result_null(sql_context *); > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index abed46486..0c86ed0f3 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -501,7 +501,7 @@ void sqlVdbeMemShallowCopy(Mem *, const Mem *, int); > void sqlVdbeMemMove(Mem *, Mem *); > int sqlVdbeMemNulTerminate(Mem *); > int sqlVdbeMemSetStr(Mem *, const char *, int, u8, void (*)(void *)); > -void sqlVdbeMemSetInt64(Mem *, i64); > +void sqlVdbeMemSetInt64(Mem *, i64, bool is_neg); 4. That function does not exist. And looks like its existence has ceased not in this commit, but in one of the previous ones. Please, drop. > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c > index 393782c23..752efeecb 100644 > --- a/src/box/sql/vdbeapi.c > +++ b/src/box/sql/vdbeapi.c > @@ -239,6 +239,16 @@ sql_value_int64(sql_value * pVal) > return i; > } > > +uint64_t > +sql_value_uint64(sql_value *val) > +{ > + int64_t i = 0; > + bool is_neg; > + sqlVdbeIntValue((struct Mem *) val, &i, &is_neg); > + assert(!is_neg); 5. Why did not you add a similar assertion to sql_value_int64()? > + return i; > +} > + > enum sql_subtype > sql_value_subtype(sql_value * pVal) > { > diff --git a/test/sql-tap/fkey2.test.lua b/test/sql-tap/fkey2.test.lua > index 54e5059b3..8e786edec 100755 > --- a/test/sql-tap/fkey2.test.lua > +++ b/test/sql-tap/fkey2.test.lua > @@ -320,10 +320,10 @@ test:do_execsql_test( > CREATE TABLE j(j INT PRIMARY KEY REFERENCES i); > INSERT INTO i VALUES(35); > INSERT INTO j VALUES(35); > - SELECT j, typeof(j) FROM j; > + SELECT j FROM j; > ]], { > -- > - 35, "integer" > + 35 6. Why did you drop 'typeof' instead of having it fixed? > -- > }) > > diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua > index 314c528ab..9cf9c2f67 100755 > --- a/test/sql-tap/func.test.lua > +++ b/test/sql-tap/func.test.lua > @@ -1742,10 +1742,20 @@ test:do_catchsql_test( > SELECT abs(-9223372036854775807-1); > ]], { > -- > - 1, "Failed to execute SQL statement: integer overflow" > + 0, {9223372036854775808LL} > -- > }) > > +test:do_catchsql_test( > + "func-18.33", > + [[ > + SELECT abs(-9223372036854775807-1); > + ]], { > + -- > + 0, {9223372036854775808LL} > + -- > +}) 7. But this test is exactly the same as the previous one. Why do you need it? Consider my review fixes here and on the branch in a separate commit. ============================================================= diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 365f8f9d2..a4f513ae8 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -184,12 +184,12 @@ absFunc(sql_context * context, int argc, sql_value ** argv) sql_result_uint(context, sql_value_uint64(argv[0])); break; } - case MP_INT:{ - i64 iVal = sql_value_int64(argv[0]); - assert(iVal < 0); - sql_result_uint(context, -iVal); + case MP_INT: { + int64_t value = sql_value_int64(argv[0]); + assert(value < 0); + sql_result_uint(context, -value); break; - } + } case MP_NIL:{ /* IMP: R-37434-19929 Abs(X) returns NULL if X is NULL. */ sql_result_null(context);