[tarantool-patches] Re: [PATCH 4/6] sql: make built-in functions operate on unsigned values

[Vladislav Shpilevoy <v.shpilevoy@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);