[tarantool-patches] Re: [PATCH 02/13] sql: make VDBE recognize big integers

[Stanislav Zudin <szudin@tarantool.org>]

On 25.03.2019 18:11, n.pettik wrote:
> 
> 
>> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
>>
>> VDBE distinguishes signed and unsigned integers.
>> SELECT query correctly returns values greater
>> than INT64_MAX.
> 
> Please, add tests on these changes.
> 
>> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
>> index ee20faab7..cb2927144 100644
>> --- a/src/box/lua/sql.c
>> +++ b/src/box/lua/sql.c
>> @@ -32,7 +32,10 @@ lua_push_row(struct lua_State *L, struct sql_stmt *stmt)
>> 		int type = sql_column_type(stmt, i);
>> 		switch (type) {
>> 		case SQL_INTEGER:
>> -			luaL_pushint64(L, sql_column_int64(stmt, i));
>> +			if (sql_column_is_unsigned(stmt, i))
>> +				luaL_pushuint64(L, sql_column_int64(stmt, i));
>> +			else
>> +				luaL_pushint64(L, sql_column_int64(stmt, i));
>> 			break;
>> 		case SQL_FLOAT:
>> 			lua_pushnumber(L, sql_column_double(stmt, i));
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index 0ef373c24..92e2f282f 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -586,6 +586,9 @@ sql_column_text(sql_stmt *,
>> int
>> sql_column_type(sql_stmt *, int iCol);
>>
>> +bool
>> +sql_column_is_unsigned(sql_stmt *, int iCol);
>> +
>> sql_value *
>> sql_column_value(sql_stmt *,
>> 		     int iCol);
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index c87b10757..ea398e7d6 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -1141,6 +1141,8 @@ case OP_Int64: {           /* out2 */
>> 	pOut = out2Prerelease(p, pOp);
>> 	assert(pOp->p4.pI64!=0);
>> 	pOut->u.i = *pOp->p4.pI64;
>> +	if (pOp->p4type == P4_UINT64)
>> +		pOut->flags = MEM_Int | MEM_Unsigned;
> 
> Why combination of flags? Doesn’t Unsigned implie that it is integer?

It caused less changes in VDBE.
The 2-nd version of the patch handles 'unsigned' as independent type.

> 
>> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
>> index f9bb96f09..d4383901c 100644
>> --- a/src/box/sql/vdbe.h
>> +++ b/src/box/sql/vdbe.h
>> @@ -70,7 +70,7 @@ struct VdbeOp {
>> 		int i;		/* Integer value if p4type==P4_INT32 */
>> 		void *p;	/* Generic pointer */
>> 		char *z;	/* Pointer to data for string (char array) types */
>> -		i64 *pI64;	/* Used when p4type is P4_INT64 */
>> +		i64 *pI64;	/* Used when p4type is P4_INT64 or P4_UINT64 */
> 
> Please, add a separate member to union.
> 
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
>> index 61b7d58b2..66b21299a 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -248,6 +248,9 @@ struct Mem {
>> #define MEM_Agg       0x4000	/* Mem.z points to an agg function context */
>> #define MEM_Zero      0x8000	/* Mem.i contains count of 0s appended to blob */
>> #define MEM_Subtype   0x10000	/* Mem.eSubtype is valid */
>> +#define MEM_Unsigned  0x20000	/* Value is unsigned integer.
>> +				 * Combine this flag with MEM_Int
>> +				 * if necessary */
>> #ifdef SQL_OMIT_INCRBLOB
>> #undef MEM_Zero
>> #define MEM_Zero 0x0000
> 
> Garbage I guess. Otherwise, please describe this change.

??? Which one?

> 
>> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
>> index d7e89073e..02a1b2e0f 100644
>> --- a/src/box/sql/vdbeapi.c
>> +++ b/src/box/sql/vdbeapi.c
>> @@ -1044,6 +1044,14 @@ sql_column_type(sql_stmt * pStmt, int i)
>> 	return iType;
>> }
>>
>> +bool sql_column_is_unsigned(sql_stmt * pStmt, int i)
> 
> Bad formatting. Should be:
> 
> bool
> sql_column_is_unsigned...

This function was removed.

> 
>> +{
>> +	const struct Mem* pMem = columnMem(pStmt, i);
>> +	if (!pMem)
>> +		return false;
> 
> Is this check necessary? Other column-like function from this
> set don’t provide it.
> 
>> +	return (pMem->flags & MEM_Unsigned);
>> +}
>> +
>> enum sql_subtype
>> sql_column_subtype(struct sql_stmt *stmt, int i)
>> {
> 
>