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

[n.pettik]

>> 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:

What kind of mess do you mean? Here we have two functions:
one sets unsigned result, another one - signed. Pretty straight
approach, isn’t it? What is more, this is the only place where
branching is required.

>    /* 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?

Because mem_set_int() accepts int64 and to avoid
extra casts to uint (since almost al used values are singed).

> 3. Please, move 'result_uint' above 'result_int' to
> avoid messing diff with 'result_bool’.

… Ok, done.

>> 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.

Removed in previous patch.

>> 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()?

Because it can accept both signed and unsigned values.

>> 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?

Because typeof() function will be fixed: it should return
type of column, not type of particular value. To avoid
abusing test files, I decided to drop it (since it is going
to change again soon).

>> 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?

Probably I wanted to do this:

diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 4d96f2fed..3ff623e82 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1749,10 +1749,10 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "func-18.33",
     [[
-        SELECT abs(-9223372036854775807-1);
+        SELECT abs(-9223372036854775807-2);
     ]], {
         -- <func-18.32>
-        0, {9223372036854775808LL}
+        1, "Failed to execute SQL statement: integer is overflowed"
         -- </func-18.32>
 })

> 
> Consider my review fixes here and on the branch in a
> separate commit.

Applied.