From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Nikita Pettik <korablev@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 4/6] sql: make built-in functions operate on unsigned values Date: Tue, 11 Jun 2019 23:11:37 +0200 [thread overview] Message-ID: <4cbc3612-b655-8b2b-d879-6b1445c9535b@tarantool.org> (raw) In-Reply-To: <4acf9fab112e7ca119f1b2bf5efea53b3831ea4f.1559919361.git.korablev@tarantool.org> 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; > ]], { > -- <fkey2-2.1> > - 35, "integer" > + 35 6. Why did you drop 'typeof' instead of having it fixed? > -- </fkey2-2.1> > }) > > 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); > ]], { > -- <func-18.32> > - 1, "Failed to execute SQL statement: integer overflow" > + 0, {9223372036854775808LL} > -- </func-18.32> > }) > > +test:do_catchsql_test( > + "func-18.33", > + [[ > + SELECT abs(-9223372036854775807-1); > + ]], { > + -- <func-18.32> > + 0, {9223372036854775808LL} > + -- </func-18.32> > +}) 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);
next prev parent reply other threads:[~2019-06-11 21:11 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-07 15:37 [tarantool-patches] [PATCH 0/6] Introduce UNSIGNED type in SQL Nikita Pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 1/6] sql: refactor sql_atoi64() Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:20 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:32 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:33 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 12:24 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 4/6] sql: make built-in functions operate on unsigned values Nikita Pettik 2019-06-11 21:11 ` Vladislav Shpilevoy [this message] 2019-07-01 14:21 ` [tarantool-patches] " n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 5/6] sql: introduce extended range for INTEGER type Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-24 15:59 ` Konstantin Osipov 2019-07-24 16:54 ` n.pettik 2019-07-24 17:09 ` Konstantin Osipov 2019-06-07 15:37 ` [tarantool-patches] [PATCH 6/6] sql: allow to specify UNSIGNED column type Nikita Pettik 2019-07-01 21:53 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-11 21:25 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-07-18 20:56 ` n.pettik 2019-07-18 21:08 ` Vladislav Shpilevoy 2019-07-18 21:13 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-22 19:17 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-17 0:54 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-08-06 19:36 ` n.pettik 2019-07-24 13:01 ` [tarantool-patches] Re: [PATCH 0/6] Introduce UNSIGNED type in SQL Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4cbc3612-b655-8b2b-d879-6b1445c9535b@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 4/6] sql: make built-in functions operate on unsigned values' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox