[tarantool-patches] Re: [PATCH 04/13] sql: support big integers within sql binding

[Stanislav Zudin <szudin@tarantool.org>]

On 25.03.2019 18:12, n.pettik wrote:
> 
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 7c77df2e5..31b89a75e 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -130,13 +130,8 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
>> 	switch (mp_typeof(**packet)) {
>> 	case MP_UINT: {
>> 		uint64_t n = mp_decode_uint(packet);
>> -		if (n > INT64_MAX) {
>> -			diag_set(ClientError, ER_SQL_BIND_VALUE,
>> -				 sql_bind_name(bind), "INTEGER");
>> -			return -1;
>> -		}
>> 		bind->i64 = (int64_t) n;
> 
> Please, add to bind struct separate member to hold uints.

Done.

> 
>> -		bind->type = SQL_INTEGER;
>> +		bind->type = (n > INT64_MAX) ? SQL_UNSIGNED : SQL_INTEGER;
>> 		bind->bytes = sizeof(bind->i64);
>> 		break;
>> 	}
>> @@ -246,6 +241,7 @@ sql_column_to_messagepack(struct sql_stmt *stmt, int i,
>> 	int type = sql_column_type(stmt, i);
>> 	switch (type) {
>> 	case SQL_INTEGER: {
>> +		assert(!sql_column_is_unsigned(stmt, i));
>> 		int64_t n = sql_column_int64(stmt, i);
>> 		if (n >= 0)
>> 			size = mp_sizeof_uint(n);
>> @@ -260,6 +256,16 @@ sql_column_to_messagepack(struct sql_stmt *stmt, int i,
>> 			mp_encode_int(pos, n);
>> 		break;
>> 	}
>> +	case SQL_UNSIGNED: {
>> +		assert(sql_column_is_unsigned(stmt, i));
>> +		int64_t n = sql_column_int64(stmt, i);
> 
> ->sql_column_uint64()
> 
Done.

>> +		size = mp_sizeof_uint(n);
>> +		char *pos = (char *) region_alloc(region, size);
>> +		if (pos == NULL)
>> +			goto oom;
>> +		mp_encode_uint(pos, n);
>> +		break;
>> +	}
>>
>> diff --git a/src/box/lua/lua_sql.c b/src/box/lua/lua_sql.c
>> index f5a7b7819..57a3161c7 100644
>> --- a/src/box/lua/lua_sql.c
>> +++ b/src/box/lua/lua_sql.c
>> @@ -60,6 +60,9 @@ lua_sql_call(sql_context *pCtx, int nVal, sql_value **apVal) {
>> 		case SQL_INTEGER:
>> 			luaL_pushint64(L, sql_value_int64(param));
>> 			break;
>> +		case SQL_UNSIGNED:
>> +			luaL_pushuint64(L, sql_value_int64(param));
> 
> Let’s introduce sql_value_uint64().

Done.

> 
>> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
>> index cb2927144..9a35c03aa 100644
>> --- a/src/box/lua/sql.c
>> +++ b/src/box/lua/sql.c
>> @@ -32,10 +32,12 @@ lua_push_row(struct lua_State *L, struct sql_stmt *stmt)
>> 		int type = sql_column_type(stmt, i);
>> 		switch (type) {
>> 		case SQL_INTEGER:
>> -			if (sql_column_is_unsigned(stmt, i))
>> -				luaL_pushuint64(L, sql_column_int64(stmt, i));
>> -			else
>> -				luaL_pushint64(L, sql_column_int64(stmt, i));
>> +			assert(!sql_column_is_unsigned(stmt, i));
>> +			luaL_pushint64(L, sql_column_int64(stmt, i));
>> +			break;
>> +		case SQL_UNSIGNED:
>> +			assert(sql_column_is_unsigned(stmt, i));
>> +			luaL_pushuint64(L, sql_column_int64(stmt, i));
>> 			break;
>> 		case SQL_FLOAT:
>> 			lua_pushnumber(L, sql_column_double(stmt, i));
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index 21a69aa51..cf65bf2a2 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>>
>> @@ -191,6 +192,11 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
>> 			sql_result_int64(context, iVal);
>> 			break;
>> 		}
>> +	case SQL_UNSIGNED: {
>> +			i64 iVal = sql_value_int64(argv[0]);
> 
> Replace this with uints once you add sql_value_uint64().
> 
>> +			sql_result_int64(context, iVal);
> 
> The same: lets add sql_result_uint64().
> 
>> +			break;
>> +		}
>> 	case SQL_NULL:{
>> 			/* IMP: R-37434-19929 Abs(X) returns NULL if X is NULL. */
>> 			sql_result_null(context);
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index 92e2f282f..56aa7c681 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> /**
>>
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index ea398e7d6..8a7f7a12f 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -1439,7 +1439,7 @@ case OP_IntCopy: {            /* out2 */
>> 	pIn1 = &aMem[pOp->p1];
>> 	assert((pIn1->flags & MEM_Int)!=0);
>> 	pOut = &aMem[pOp->p2];
>> -	sqlVdbeMemSetInt64(pOut, pIn1->u.i);
>> +	sqlVdbeMemSetInt64(pOut, pIn1->u.i, (pIn1->flags & MEM_Unsigned)!=0);
>> 	break;
>> }
>>
>> @@ -1770,7 +1770,7 @@ integer_overflow:
>> case OP_CollSeq: {
>> 	assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl == NULL);
>> 	if (pOp->p1) {
>> -		sqlVdbeMemSetInt64(&aMem[pOp->p1], 0);
>> +		sqlVdbeMemSetInt64(&aMem[pOp->p1], 0, false);
> 
> Instead of passing argument indicating signedness, I would
> introduce separate function to set vdbe memory with uint value.

Done.

> 
>> 	}
>> 	break;
>> }
>> @@ -5317,7 +5317,7 @@ case OP_AggStep: {
>> 	if (pCtx->skipFlag) {
>> 		assert(pOp[-1].opcode==OP_CollSeq);
>> 		i = pOp[-1].p1;
>> -		if (i) sqlVdbeMemSetInt64(&aMem[i], 1);
>> +		if (i) sqlVdbeMemSetInt64(&aMem[i], 1, false);
>> 	}
>> 	break;
>> }
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
>> index 66b21299a..0375845d9 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -477,7 +477,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);
>> #ifdef SQL_OMIT_FLOATING_POINT
>> #define sqlVdbeMemSetDouble sqlVdbeMemSetInt64
>> #else
>> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
>> index 02a1b2e0f..2c486552e 100644
>> --- a/src/box/sql/vdbeapi.c
>> +++ b/src/box/sql/vdbeapi.c
>> @@ -279,8 +279,49 @@ sql_value_type(sql_value * pVal)
>> 		SQL_NULL,	/* 0x1d */
>> 		SQL_INTEGER,	/* 0x1e */
>> 		SQL_NULL,	/* 0x1f */
>> +
>> +		SQL_BLOB,	/* 0x20 */
>> +		SQL_NULL,	/* 0x21 */
>> +		SQL_TEXT,	/* 0x22 */
>> +		SQL_NULL,	/* 0x23 */
>> +		SQL_UNSIGNED,	/* 0x24 */
>> +		SQL_NULL,	/* 0x25 */
>> +		SQL_UNSIGNED,	/* 0x26 */
>> +		SQL_NULL,	/* 0x27 */
>> +		SQL_FLOAT,	/* 0x28 */
>> +		SQL_NULL,	/* 0x29 */
>> +		SQL_FLOAT,	/* 0x2a */
>> +		SQL_NULL,	/* 0x2b */
>> +		SQL_INTEGER,	/* 0x2c */
>> +		SQL_NULL,	/* 0x2d */
>> +		SQL_INTEGER,	/* 0x2e */
>> +		SQL_NULL,	/* 0x2f */
>> +		SQL_BLOB,	/* 0x30 */
>> +		SQL_NULL,	/* 0x31 */
>> +		SQL_TEXT,	/* 0x32 */
>> +		SQL_NULL,	/* 0x33 */
>> +		SQL_INTEGER,	/* 0x34 */
>> +		SQL_NULL,	/* 0x35 */
>> +		SQL_INTEGER,	/* 0x36 */
>> +		SQL_NULL,	/* 0x37 */
>> +		SQL_FLOAT,	/* 0x38 */
>> +		SQL_NULL,	/* 0x39 */
>> +		SQL_FLOAT,	/* 0x3a */
>> +		SQL_NULL,	/* 0x3b */
>> +		SQL_INTEGER,	/* 0x3c */
>> +		SQL_NULL,	/* 0x3d */
>> +		SQL_INTEGER,	/* 0x3e */
>> +		SQL_NULL,	/* 0x3f */
> 
> Looks terrible. Could we rework this mechanism of fetching type?
> Soon we are going to add several types more, so the size of array
> will become enormous.
> 

Done.


>> 	};
>> -	return aType[pVal->flags & MEM_PURE_TYPE_MASK];
>> +
>> +	assert(MEM_Unsigned == 0x20000);
>> +	/* compress the unsigned bit with the pure
>> +	 * type bits, to make them applicable for
>> +	 * array indexing.
>> +	 */
>> +	u32 offset = (pVal->flags >> 12) | (pVal->flags & MEM_PURE_TYPE_MASK);
>> +	assert(offset < 0x40);
>> +	return aType[offset];
>> }
>>
>> /* Make a copy of an sql_value object
>> @@ -401,13 +442,13 @@ sql_result_error(sql_context * pCtx, const char *z, int n)
>>
>> void
>> @@ -1370,7 +1411,20 @@ sql_bind_int64(sql_stmt * pStmt, int i, sql_int64 iValue)
>> 	rc = vdbeUnbind(p, i);
>> 	if (rc == SQL_OK) {
>> 		rc = sql_bind_type(p, i, "INTEGER");
>> -		sqlVdbeMemSetInt64(&p->aVar[i - 1], iValue);
>> +		sqlVdbeMemSetInt64(&p->aVar[i - 1], iValue, false);
>> +	}
>> +	return rc;
>> +}
>> +
>> +int
>> +sql_bind_uint64(sql_stmt * pStmt, int i, sql_uint64 iValue)
>> +{
>> +	int rc;
>> +	Vdbe *p = (Vdbe *) pStmt;
>> +	rc = vdbeUnbind(p, i);
>> +	if (rc == SQL_OK) {
>> +		rc = sql_bind_type(p, i, "INTEGER”);
> 
> Why integer? I guess it should be unsigned.

Fixed.

> 
>> +		sqlVdbeMemSetInt64(&p->aVar[i - 1], (u64)iValue, true);
>> 	}
>> 	return rc;
>> }
>>
> 
>