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

["n.pettik" <korablev@tarantool.org>]

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

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

> +		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().

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

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

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

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