[tarantool-patches] Re: [PATCH 4/6] sql: make built-in functions operate on unsigned values
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jun 12 00:11:37 MSK 2019
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);
More information about the Tarantool-patches
mailing list