[PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type

[PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type

[Stanislav Zudin]

The patch enables support of big integers in the range [2^63, 2^64-1].
Conversion functions use return value to inform about value
they return - signed integer in the range [-2^63,2^63-1] or 
unsigned integer in the range [2^63, 2^64 -1].
The changes affect sql processing, VDBE, arithmetic functions and
aggregate functions.

Issue: https://github.com/tarantool/tarantool/issues/3810
Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3810-sql-uint64-support

Stanislav Zudin (13):
  sql: Convert big integers from string
  sql: make VDBE recognize big integers
  sql: removes unused function.
  sql: support big integers within sql binding
  sql: removes redundant function.
  sql: aux functions to support big integers
  sql: arithmetic functions support big integers
  sql: aggregate sql functions support big int
  sql: fixes errors
  sql: fixes an error in sqlSubInt64
  sql: fixes an error in string to int64 conversion
  sql: fixes an error in uint64 to double casting
  sql: support -2^63 .. 2^64-1 integer type

 src/box/execute.c                        |  19 +-
 src/box/lua/lua_sql.c                    |   3 +
 src/box/lua/sql.c                        |   3 +
 src/box/sql/build.c                      |   4 +-
 src/box/sql/expr.c                       |  26 +-
 src/box/sql/func.c                       |  42 ++-
 src/box/sql/main.c                       |  16 -
 src/box/sql/sqlInt.h                     |  91 +++--
 src/box/sql/util.c                       | 401 +++++++++++++++--------
 src/box/sql/vdbe.c                       |  65 ++--
 src/box/sql/vdbe.h                       |   3 +-
 src/box/sql/vdbeInt.h                    |   9 +-
 src/box/sql/vdbeapi.c                    |  73 ++++-
 src/box/sql/vdbeaux.c                    |  12 +-
 src/box/sql/vdbemem.c                    |  17 +-
 test/sql-tap/func.test.lua               |   6 +-
 test/sql-tap/hexlit.test.lua             |   6 +-
 test/sql/gh-2347-max-int-literals.result |   2 +-
 test/sql/integer-overflow.result         |  10 +-
 test/sql/iproto.result                   |  11 +-
 test/sql/iproto.test.lua                 |   5 +-
 21 files changed, 554 insertions(+), 270 deletions(-)

-- 
2.17.1

[PATCH 01/13] sql: Convert big integers from string

[Stanislav Zudin]

Correctly handles the signed and unsigned integers on the way from sql
expression to VDBE.
VDBE doesn't distinguish yet signed and unsigned integers.
---
 src/box/sql/expr.c   |  23 ++++---
 src/box/sql/main.c   |   2 +-
 src/box/sql/sqlInt.h |  63 +++++++++++------
 src/box/sql/util.c   | 160 +++++++++++++++++++------------------------
 src/box/sql/vdbe.c   |   2 +-
 5 files changed, 125 insertions(+), 125 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 82688dff3..3fbfab7a0 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3335,23 +3335,24 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg,
 		int64_t value;
 		const char *z = expr->u.zToken;
 		assert(z != NULL);
-		int c = sql_dec_or_hex_to_i64(z, &value);
-		if (c == 1 || (c == 2 && !is_neg) ||
-		    (is_neg && value == SMALLEST_INT64)) {
+		enum atoi_result c = sql_dec_or_hex_to_i64(z, is_neg, &value);
+		switch(c) {
+		case ATOI_OVERFLOW:
 			if (sql_strnicmp(z, "0x", 2) == 0) {
 				sqlErrorMsg(parse,
-						"hex literal too big: %s%s",
-						is_neg ? "-" : "", z);
+					    "hex literal too big: %s%s",
+					    is_neg ? "-" : "", z);
 			} else {
 				sqlErrorMsg(parse,
-						"oversized integer: %s%s",
-						is_neg ? "-" : "", z);
+					    "oversized integer: %s%s",
+					    is_neg ? "-" : "", z);
 			}
-		} else {
-			if (is_neg)
-				value = c == 2 ? SMALLEST_INT64 : -value;
+			break;
+		case ATOI_UNSIGNED:
+		case ATOI_SIGNED:
 			sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
-					      (u8 *)&value, P4_INT64);
+					  (u8 *)&value, P4_INT64);
+			break;
 		}
 	}
 }
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 0b3bd201e..9fe2e2c9d 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1920,7 +1920,7 @@ sql_uri_int64(const char *zFilename,	/* Filename as passed to xOpen */
 {
 	const char *z = sql_uri_parameter(zFilename, zParam);
 	int64_t v;
-	if (z != NULL && sql_dec_or_hex_to_i64(z, &v) == 0)
+	if (z != NULL && sql_dec_or_hex_to_i64(z, false, &v) == 0)
 		bDflt = v;
 	return bDflt;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index eb1488576..0ef373c24 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4274,21 +4274,37 @@ enum field_type *
 field_type_sequence_dup(struct Parse *parse, enum field_type *types,
 			uint32_t len);
 
+enum atoi_result {
+	/** Successful transformation.
+	 * Fits in a 64-bit signed integer.
+	 */
+	ATOI_SIGNED = 0,
+	/** Integer is too large for a 64-bit
+	 * unsigned integer or is malformed
+	 */
+	ATOI_OVERFLOW = 1,
+	/** Successful transformation.
+	 * Fits in a 64-bit unsigned integer.
+	 */
+	ATOI_UNSIGNED = 2
+};
+
+
 /**
- * Convert z to a 64-bit signed integer.  z must be decimal. This
- * routine does *not* accept hexadecimal notation.
+ * Converts z to a 64-bit signed or unsigned integer.
+ * z must be decimal. This routine does *not* accept
+ * hexadecimal notation.
  *
  * If the z value is representable as a 64-bit twos-complement
- * integer, then write that value into *val and return 0.
+ * integer, then write that value into *val and return ATOI_SIGNED.
  *
- * If z is exactly 9223372036854775808, return 2.  This special
- * case is broken out because while 9223372036854775808 cannot be
- * a signed 64-bit integer, its negative -9223372036854775808 can
- * be.
+ * If z is a number in the range
+ * [9223372036854775808, 18446744073709551615] function returns
+ * ATOI_UNSIGNED and result must be treated as unsigned.
  *
  * If z is too big for a 64-bit integer and is not
  * 9223372036854775808  or if z contains any non-numeric text,
- * then return 1.
+ * then return ATOI_OVERFLOW.
  *
  * length is the number of bytes in the string (bytes, not
  * characters). The string is not necessarily zero-terminated.
@@ -4298,13 +4314,14 @@ field_type_sequence_dup(struct Parse *parse, enum field_type *types,
  * @param[out] val Output integer value.
  * @param length String length in bytes.
  * @retval
- *     0    Successful transformation.  Fits in a 64-bit signed
- *          integer.
- *     1    Integer too large for a 64-bit signed integer or is
- *          malformed
- *     2    Special case of 9223372036854775808
- */
-int
+ *     ATOI_SIGNED	Successful transformation.
+ *     			Fits in a 64-bit signed integer
+ *     ATOI_OVERFLOW    Integer too large for a 64-bit
+ *     			unsigned integer or is malformed
+ *     ATOI_UNSIGNED    Successful transformation.
+ *     			Fits in a 64-bit signed integer
+ */
+enum atoi_result
 sql_atoi64(const char *z, int64_t *val, int length);
 
 /**
@@ -4313,14 +4330,18 @@ sql_atoi64(const char *z, int64_t *val, int length);
  * accepts hexadecimal literals, whereas sql_atoi64() does not.
  *
  * @param z Literal being parsed.
+ * @param is_neg Sign of the number being converted
  * @param[out] val Parsed value.
  * @retval
- *     0    Successful transformation.  Fits in a 64-bit signed integer.
- *     1    Integer too large for a 64-bit signed integer or is malformed
- *     2    Special case of 9223372036854775808
- */
-int
-sql_dec_or_hex_to_i64(const char *z, int64_t *val);
+ *     ATOI_SIGNED	Successful transformation.
+ *     			Fits in a 64-bit signed integer
+ *     ATOI_OVERFLOW    Integer too large for a 64-bit
+ *     			unsigned integer or is malformed
+ *     ATOI_UNSIGNED    Successful transformation.
+ *     			Fits in a 64-bit signed integer
+ */
+enum atoi_result
+sql_dec_or_hex_to_i64(const char *z, bool is_neg, int64_t *val);
 
 void sqlErrorWithMsg(sql *, int, const char *, ...);
 void sqlError(sql *, int);
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index c89e2e8ab..be77f72f8 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -591,110 +591,56 @@ sqlAtoF(const char *z, double *pResult, int length)
 #endif				/* SQL_OMIT_FLOATING_POINT */
 }
 
-/*
- * Compare the 19-character string zNum against the text representation
- * value 2^63:  9223372036854775808.  Return negative, zero, or positive
- * if zNum is less than, equal to, or greater than the string.
- * Note that zNum must contain exactly 19 characters.
- *
- * Unlike memcmp() this routine is guaranteed to return the difference
- * in the values of the last digit if the only difference is in the
- * last digit.  So, for example,
- *
- *      compare2pow63("9223372036854775800", 1)
- *
- * will return -8.
- */
-static int
-compare2pow63(const char *zNum, int incr)
-{
-	int c = 0;
-	int i;
-	/* 012345678901234567 */
-	const char *pow63 = "922337203685477580";
-	for (i = 0; c == 0 && i < 18; i++) {
-		c = (zNum[i * incr] - pow63[i]) * 10;
-	}
-	if (c == 0) {
-		c = zNum[18 * incr] - '8';
-		testcase(c == (-1));
-		testcase(c == 0);
-		testcase(c == (+1));
-	}
-	return c;
-}
+#ifndef INT64_MIN_MOD
+/* Modulo of INT64_MIN */
+#define INT64_MIN_MOD 0x8000000000000000
+#endif
 
-int
+enum atoi_result
 sql_atoi64(const char *z, int64_t *val, int length)
 {
-	int incr = 1;
-	u64 u = 0;
 	int neg = 0;		/* assume positive */
-	int i;
-	int c = 0;
-	int nonNum = 0;		/* True if input contains UTF16 with high byte non-zero */
-	const char *zStart;
 	const char *zEnd = z + length;
-	incr = 1;
+	int incr = 1;
 	while (z < zEnd && sqlIsspace(*z))
 		z += incr;
-	if (z < zEnd) {
-		if (*z == '-') {
-			neg = 1;
-			z += incr;
-		} else if (*z == '+') {
-			z += incr;
-		}
-	}
-	zStart = z;
-	/* Skip leading zeros. */
-	while (z < zEnd && z[0] == '0') {
+
+	if (z >= zEnd)
+		return ATOI_OVERFLOW; /* invalid format */
+	if (*z == '-') {
+		neg = 1;
 		z += incr;
 	}
-	for (i = 0; &z[i] < zEnd && (c = z[i]) >= '0' && c <= '9';
-	     i += incr) {
-		u = u * 10 + c - '0';
-	}
-	if (u > LARGEST_INT64) {
-		*val = neg ? SMALLEST_INT64 : LARGEST_INT64;
-	} else if (neg) {
-		*val = -(i64) u;
+
+	char* end = NULL;
+	u64 u = strtoull(z, &end, 10);
+	if (end == z)
+		return ATOI_OVERFLOW;
+	if (errno == ERANGE)
+		return ATOI_OVERFLOW;
+
+	enum atoi_result rc;
+	if (neg) {
+		rc = ATOI_SIGNED;
+		if (u <= INT64_MAX)
+			*val = -u;
+		else if (u == INT64_MIN_MOD)
+			*val = (i64) u;
+		else
+			rc = ATOI_OVERFLOW;
 	} else {
 		*val = (i64) u;
+		rc = (u <= INT64_MAX) ? ATOI_SIGNED
+				      : ATOI_UNSIGNED;
 	}
-	if (&z[i] < zEnd || (i == 0 && zStart == z) || i > 19 * incr ||
-	    nonNum) {
-		/* zNum is empty or contains non-numeric text or is longer
-		 * than 19 digits (thus guaranteeing that it is too large)
-		 */
-		return 1;
-	} else if (i < 19 * incr) {
-		/* Less than 19 digits, so we know that it fits in 64 bits */
-		assert(u <= LARGEST_INT64);
-		return 0;
-	} else {
-		/* zNum is a 19-digit numbers.  Compare it against 9223372036854775808. */
-		c = compare2pow63(z, incr);
-		if (c < 0) {
-			/* zNum is less than 9223372036854775808 so it fits */
-			assert(u <= LARGEST_INT64);
-			return 0;
-		} else if (c > 0) {
-			/* zNum is greater than 9223372036854775808 so it overflows */
-			return 1;
-		} else {
-			/* zNum is exactly 9223372036854775808.  Fits if negative.  The
-			 * special case 2 overflow if positive
-			 */
-			assert(u - 1 == LARGEST_INT64);
-			return neg ? 0 : 2;
-		}
-	}
+
+	return rc;
 }
 
-int
-sql_dec_or_hex_to_i64(const char *z, int64_t *val)
+enum atoi_result
+sql_dec_or_hex_to_i64(const char *z, bool is_neg, int64_t *val)
 {
+	enum atoi_result rc;
 	if (z[0] == '0' && (z[1] == 'x' || z[1] == 'X')) {
 		uint64_t u = 0;
 		int i, k;
@@ -702,9 +648,41 @@ sql_dec_or_hex_to_i64(const char *z, int64_t *val)
 		for (k = i; sqlIsxdigit(z[k]); k++)
 			u = u * 16 + sqlHexToInt(z[k]);
 		memcpy(val, &u, 8);
-		return (z[k] == 0 && k - i <= 16) ? 0 : 1;
+
+		/* Determine result */
+		if ((k - i) > 16)
+			rc = ATOI_OVERFLOW;
+		else if (u > INT64_MAX)
+			rc = ATOI_UNSIGNED;
+		else
+			rc = ATOI_SIGNED;
+	}
+	else
+		rc = sql_atoi64(z, val, sqlStrlen30(z));
+
+	/* Apply sign */
+	if (is_neg) {
+		switch (rc) {
+		case ATOI_SIGNED:
+			*val = -*val;
+			break;
+		case ATOI_OVERFLOW:
+			/* n/a */
+			break;
+		case ATOI_UNSIGNED:
+			/* A special processing is required
+			 * for the INT64_MIN value. Any other
+			 * values can't be presented as signed,
+			 * so change the return value to error. */
+			if (*val == INT64_MIN)
+				rc = ATOI_SIGNED;
+			else
+				rc = ATOI_OVERFLOW;
+			break;
+		}
 	}
-	return sql_atoi64(z, val, sqlStrlen30(z));
+
+	return rc;
 }
 
 /*
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c1da9a4aa..c87b10757 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -410,7 +410,7 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
 	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
 	if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0)
 		return 0;
-	if (sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)==SQL_OK)
+	if (sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)==ATOI_SIGNED)
 		return MEM_Int;
 	return MEM_Real;
 }
-- 
2.17.1

[tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string

[n.pettik]

> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> Correctly handles the signed and unsigned integers on the way from sql
> expression to VDBE.
> VDBE doesn't distinguish yet signed and unsigned integers.

Firstly, please add tag to each patch related to patch-set:

Part of #….

Or

Needed for #…

Secondly, several tests fail on this patch:
Failed tasks:
- [sql-tap/cast.test.lua, memtx]
- [sql-tap/tkt-a8a0d2996a.test.lua, memtx]
- [sql-tap/misc3.test.lua, memtx]
- [sql-tap/in4.test.lua, memtx]

sql/gh-2347-max-int-literals.test.lua           vinyl           [ fail ]

AFAIK we follow policy stating that each separate patch
shouldn’t break existing tests. Mb you should reconsider
structure of your patch-set.

> src/box/sql/expr.c   |  23 ++++---
> src/box/sql/main.c   |   2 +-
> src/box/sql/sqlInt.h |  63 +++++++++++------
> src/box/sql/util.c   | 160 +++++++++++++++++++------------------------
> src/box/sql/vdbe.c   |   2 +-
> 5 files changed, 125 insertions(+), 125 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 82688dff3..3fbfab7a0 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3335,23 +3335,24 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg,
> 		int64_t value;
> 		const char *z = expr->u.zToken;
> 		assert(z != NULL);
> -		int c = sql_dec_or_hex_to_i64(z, &value);
> -		if (c == 1 || (c == 2 && !is_neg) ||
> -		    (is_neg && value == SMALLEST_INT64)) {
> +		enum atoi_result c = sql_dec_or_hex_to_i64(z, is_neg, &value);

I suggest to make all atoi return -1 in case of errors (including overflow),
and 0 in case of successful conversion. Error itself you can set via diag_set().
Sign of the result you can pass via output param.
This way would be more Tarantool-like.

Moreover, instead of storing UINT in INT, I suggest to use fair enum and
output param is_uint/is_negative/is_whatever/…

Finally, please add comment explaining that unsigned value is returned
only in case when value can’t be fitted into int64 range. I don’t see
such explanation, so at the first sight code may look weird. 

> +		switch(c) {
> +		case ATOI_OVERFLOW:
> 			if (sql_strnicmp(z, "0x", 2) == 0) {
> 				sqlErrorMsg(parse,
> -						"hex literal too big: %s%s",
> -						is_neg ? "-" : "", z);
> +					    "hex literal too big: %s%s",
> +					    is_neg ? "-" : "", z);
> 			} else {
> 				sqlErrorMsg(parse,
> -						"oversized integer: %s%s",
> -						is_neg ? "-" : "", z);
> +					    "oversized integer: %s%s",
> +					    is_neg ? "-" : "", z);
> 			}
> -		} else {
> -			if (is_neg)
> -				value = c == 2 ? SMALLEST_INT64 : -value;
> +			break;
> +		case ATOI_UNSIGNED:
> +		case ATOI_SIGNED:
> 			sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
> -					      (u8 *)&value, P4_INT64);
> +					  (u8 *)&value, P4_INT64);
> +			break;
> 		}
> 	}
> }
> 
> 
> index c89e2e8ab..be77f72f8 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -591,110 +591,56 @@ sqlAtoF(const char *z, double *pResult, int length)
> #endif				/* SQL_OMIT_FLOATING_POINT */
> -int
> +enum atoi_result
> sql_atoi64(const char *z, int64_t *val, int length)
> {
> -	int incr = 1;
> -	u64 u = 0;
> 	int neg = 0;		/* assume positive */
> -	int i;
> -	int c = 0;
> -	int nonNum = 0;		/* True if input contains UTF16 with high byte non-zero */
> -	const char *zStart;
> 	const char *zEnd = z + length;
> -	incr = 1;
> +	int incr = 1;
> 	while (z < zEnd && sqlIsspace(*z))
> 		z += incr;
> -	if (z < zEnd) {
> -		if (*z == '-') {
> -			neg = 1;
> -			z += incr;
> -		} else if (*z == '+') {
> -			z += incr;
> -		}
> -	}
> -	zStart = z;
> -	/* Skip leading zeros. */
> -	while (z < zEnd && z[0] == '0') {
> +
> +	if (z >= zEnd)
> +		return ATOI_OVERFLOW; /* invalid format */

It’s not fair to return OVERFLOW result in case of “invalid format”.
See my comment concerning return values above.

> +	if (*z == '-') {
> +		neg = 1;
> 		z += incr;
> 	}
> -	for (i = 0; &z[i] < zEnd && (c = z[i]) >= '0' && c <= '9';
> -	     i += incr) {
> -		u = u * 10 + c - '0';
> -	}
> -	if (u > LARGEST_INT64) {
> -		*val = neg ? SMALLEST_INT64 : LARGEST_INT64;
> -	} else if (neg) {
> -		*val = -(i64) u;
> +
> +	char* end = NULL;

According to docs, you should set errno to 0 before
calling this function.

> +	u64 u = strtoull(z, &end, 10);
> +	if (end == z)
> +		return ATOI_OVERFLOW;

In this case, it’s not overflow but error like “"No digits were found”.

> +	if (errno == ERANGE)
> +		return ATOI_OVERFLOW;

Not only ERANGE errors can occur.
Correct way of testing on occurred errors is shown here:
https://linux.die.net/man/3/strtol

 if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN))
            || (errno != 0 && val == 0))

If these error should be caught before, then add assertions pls.

[tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string

[Stanislav Zudin]

On 25.03.2019 18:10, n.pettik wrote:
> 
> 
>> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
>>
>> Correctly handles the signed and unsigned integers on the way from sql
>> expression to VDBE.
>> VDBE doesn't distinguish yet signed and unsigned integers.
> 
> Firstly, please add tag to each patch related to patch-set:
> 
> Part of #….
> 
> Or
> 
> Needed for #…

Got it.

> 
> Secondly, several tests fail on this patch:
> Failed tasks:
> - [sql-tap/cast.test.lua, memtx]
> - [sql-tap/tkt-a8a0d2996a.test.lua, memtx]
> - [sql-tap/misc3.test.lua, memtx]
> - [sql-tap/in4.test.lua, memtx]
> 
> sql/gh-2347-max-int-literals.test.lua           vinyl           [ fail ]
> 

The final commit does pass these tests.

> AFAIK we follow policy stating that each separate patch
> shouldn’t break existing tests. Mb you should reconsider
> structure of your patch-set.
> 
>> src/box/sql/expr.c   |  23 ++++---
>> src/box/sql/main.c   |   2 +-
>> src/box/sql/sqlInt.h |  63 +++++++++++------
>> src/box/sql/util.c   | 160 +++++++++++++++++++------------------------
>> src/box/sql/vdbe.c   |   2 +-
>> 5 files changed, 125 insertions(+), 125 deletions(-)
>>
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index 82688dff3..3fbfab7a0 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -3335,23 +3335,24 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg,
>> 		int64_t value;
>> 		const char *z = expr->u.zToken;
>> 		assert(z != NULL);
>> -		int c = sql_dec_or_hex_to_i64(z, &value);
>> -		if (c == 1 || (c == 2 && !is_neg) ||
>> -		    (is_neg && value == SMALLEST_INT64)) {
>> +		enum atoi_result c = sql_dec_or_hex_to_i64(z, is_neg, &value);
> 
> I suggest to make all atoi return -1 in case of errors (including overflow),
> and 0 in case of successful conversion. Error itself you can set via diag_set().
> Sign of the result you can pass via output param.
> This way would be more Tarantool-like.

Accepted. Fixed.

> 
> Moreover, instead of storing UINT in INT, I suggest to use fair enum and
> output param is_uint/is_negative/is_whatever/…
> 
> Finally, please add comment explaining that unsigned value is returned
> only in case when value can’t be fitted into int64 range. I don’t see
> such explanation, so at the first sight code may look weird.

Done.
BTW. The declaration of atoi* function contains such explanation

> 
>> +		switch(c) {
>> +		case ATOI_OVERFLOW:
>> 			if (sql_strnicmp(z, "0x", 2) == 0) {
>> 				sqlErrorMsg(parse,
>> -						"hex literal too big: %s%s",
>> -						is_neg ? "-" : "", z);
>> +					    "hex literal too big: %s%s",
>> +					    is_neg ? "-" : "", z);
>> 			} else {
>> 				sqlErrorMsg(parse,
>> -						"oversized integer: %s%s",
>> -						is_neg ? "-" : "", z);
>> +					    "oversized integer: %s%s",
>> +					    is_neg ? "-" : "", z);
>> 			}
>> -		} else {
>> -			if (is_neg)
>> -				value = c == 2 ? SMALLEST_INT64 : -value;
>> +			break;
>> +		case ATOI_UNSIGNED:
>> +		case ATOI_SIGNED:
>> 			sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
>> -					      (u8 *)&value, P4_INT64);
>> +					  (u8 *)&value, P4_INT64);
>> +			break;
>> 		}
>> 	}
>> }
>>
>>
>> index c89e2e8ab..be77f72f8 100644
>> --- a/src/box/sql/util.c
>> +++ b/src/box/sql/util.c
>> @@ -591,110 +591,56 @@ sqlAtoF(const char *z, double *pResult, int length)
>> #endif				/* SQL_OMIT_FLOATING_POINT */
>> -int
>> +enum atoi_result
>> sql_atoi64(const char *z, int64_t *val, int length)
>> {
>> -	int incr = 1;
>> -	u64 u = 0;
>> 	int neg = 0;		/* assume positive */
>> -	int i;
>> -	int c = 0;
>> -	int nonNum = 0;		/* True if input contains UTF16 with high byte non-zero */
>> -	const char *zStart;
>> 	const char *zEnd = z + length;
>> -	incr = 1;
>> +	int incr = 1;
>> 	while (z < zEnd && sqlIsspace(*z))
>> 		z += incr;
>> -	if (z < zEnd) {
>> -		if (*z == '-') {
>> -			neg = 1;
>> -			z += incr;
>> -		} else if (*z == '+') {
>> -			z += incr;
>> -		}
>> -	}
>> -	zStart = z;
>> -	/* Skip leading zeros. */
>> -	while (z < zEnd && z[0] == '0') {
>> +
>> +	if (z >= zEnd)
>> +		return ATOI_OVERFLOW; /* invalid format */
> 
> It’s not fair to return OVERFLOW result in case of “invalid format”.
> See my comment concerning return values above.
> 
>> +	if (*z == '-') {
>> +		neg = 1;
>> 		z += incr;
>> 	}
>> -	for (i = 0; &z[i] < zEnd && (c = z[i]) >= '0' && c <= '9';
>> -	     i += incr) {
>> -		u = u * 10 + c - '0';
>> -	}
>> -	if (u > LARGEST_INT64) {
>> -		*val = neg ? SMALLEST_INT64 : LARGEST_INT64;
>> -	} else if (neg) {
>> -		*val = -(i64) u;
>> +
>> +	char* end = NULL;
> 
> According to docs, you should set errno to 0 before
> calling this function.

Good point.
Fixed.

> 
>> +	u64 u = strtoull(z, &end, 10);
>> +	if (end == z)
>> +		return ATOI_OVERFLOW;
> 
> In this case, it’s not overflow but error like “"No digits were found”.
> 
>> +	if (errno == ERANGE)
>> +		return ATOI_OVERFLOW;
> 
> Not only ERANGE errors can occur.
> Correct way of testing on occurred errors is shown here:
> https://linux.die.net/man/3/strtol
> 
>   if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN))
>              || (errno != 0 && val == 0))

IMO the (errno != 0) covers all faulty cases.

> 
> If these error should be caught before, then add assertions pls.
> 
> 

[tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/03/25 18:13]:
> I suggest to make all atoi return -1 in case of errors (including overflow),
> and 0 in case of successful conversion. Error itself you can set via diag_set().
> Sign of the result you can pass via output param.
> This way would be more Tarantool-like.

I agree.

> Moreover, instead of storing UINT in INT, I suggest to use fair enum and
> output param is_uint/is_negative/is_whatever/…

Perhaps return MP_INT/MP_UINT? 

> It’s not fair to return OVERFLOW result in case of “invalid format”.
> See my comment concerning return values above.

I agree, usign diag_set and distinct errors is a) ANSI req b)
user-friendly
 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[PATCH 02/13] sql: make VDBE recognize big integers

[Stanislav Zudin]

VDBE distinguishes signed and unsigned integers.
SELECT query correctly returns values greater
than INT64_MAX.
---
 src/box/lua/sql.c     |  5 ++++-
 src/box/sql/build.c   |  4 ++--
 src/box/sql/expr.c    |  3 +++
 src/box/sql/sqlInt.h  |  3 +++
 src/box/sql/vdbe.c    |  2 ++
 src/box/sql/vdbe.h    |  3 ++-
 src/box/sql/vdbeInt.h |  3 +++
 src/box/sql/vdbeapi.c |  8 ++++++++
 src/box/sql/vdbeaux.c | 12 +++++++-----
 src/box/sql/vdbemem.c |  2 +-
 10 files changed, 35 insertions(+), 10 deletions(-)

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/build.c b/src/box/sql/build.c
index e9851d9a1..34fd03862 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -939,10 +939,10 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
 
 	/* 5. Minimum  */
 	sqlVdbeAddOp4Dup8(v, OP_Int64, 0, first_col + 5, 0,
-			      (unsigned char*)&min_usigned_long_long, P4_INT64);
+			      (unsigned char*)&min_usigned_long_long, P4_UINT64);
 	/* 6. Maximum  */
 	sqlVdbeAddOp4Dup8(v, OP_Int64, 0, first_col + 6, 0,
-			      (unsigned char*)&max_usigned_long_long, P4_INT64);
+			      (unsigned char*)&max_usigned_long_long, P4_UINT64);
 	/* 7. Start  */
 	sqlVdbeAddOp2(v, OP_Integer, 1, first_col + 7);
 
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3fbfab7a0..fcc673436 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3349,6 +3349,9 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg,
 			}
 			break;
 		case ATOI_UNSIGNED:
+			sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
+					  (u8 *)&value, P4_UINT64);
+				break;
 		case ATOI_SIGNED:
 			sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0,
 					  (u8 *)&value, P4_INT64);
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;
 	break;
 }
 
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 */
 		double *pReal;	/* Used when p4type is P4_REAL */
 		FuncDef *pFunc;	/* Used when p4type is P4_FUNCDEF */
 		sql_context *pCtx;	/* Used when p4type is P4_FUNCCTX */
@@ -131,6 +131,7 @@ struct SubProgram {
 #define P4_INTARRAY (-12)	/* P4 is a vector of 32-bit integers */
 #define P4_SUBPROGRAM  (-13)	/* P4 is a pointer to a SubProgram structure */
 #define P4_ADVANCE  (-14)	/* P4 is a pointer to BtreeNext() or BtreePrev() */
+#define P4_UINT64   (-15)	/* P4 is a 64-bit unsigned integer */
 #define P4_FUNCCTX  (-16)	/* P4 is a pointer to an sql_context object */
 #define P4_BOOL     (-17)	/* P4 is a bool value */
 #define P4_PTR      (-18)	/* P4 is a generic pointer */
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
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)
+{
+	const struct Mem* pMem = columnMem(pStmt, i);
+	if (!pMem)
+		return false;
+	return (pMem->flags & MEM_Unsigned);
+}
+
 enum sql_subtype
 sql_column_subtype(struct sql_stmt *stmt, int i)
 {
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 0cc3c1487..37cfe0431 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -862,6 +862,7 @@ freeP4(sql * db, int p4type, void *p4)
 		}
 	case P4_REAL:
 	case P4_INT64:
+	case P4_UINT64:
 	case P4_DYNAMIC:
 	case P4_INTARRAY:{
 			sqlDbFree(db, p4);
@@ -1354,6 +1355,10 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 			sqlXPrintf(&x, "%lld", *pOp->p4.pI64);
 			break;
 		}
+	case P4_UINT64:{
+			sqlXPrintf(&x, "%ull", *pOp->p4.pI64);
+			break;
+	}
 	case P4_INT32:{
 			sqlXPrintf(&x, "%d", pOp->p4.i);
 			break;
@@ -3724,13 +3729,10 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
 	}
 	case MP_UINT: {
 		uint64_t v = mp_decode_uint(&buf);
-		if (v > INT64_MAX) {
-			diag_set(ClientError, ER_SQL_EXECUTE,
-				 "integer is overflowed");
-			return -1;
-		}
 		mem->u.i = v;
 		mem->flags = MEM_Int;
+		if (v > INT64_MAX)
+			mem->flags |= MEM_Unsigned;
 		break;
 	}
 	case MP_INT: {
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 074ff8c96..0b5ba965e 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1723,7 +1723,7 @@ mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var)
 	} else if (var->flags & MEM_Int) {
 		i = var->u.i;
 encode_int:
-		if (var->u.i >= 0)
+		if (var->u.i >= 0 || var->flags & MEM_Unsigned)
 			mpstream_encode_uint(stream, i);
 		else
 			mpstream_encode_int(stream, i);
-- 
2.17.1

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

[n.pettik]

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

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

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

> +{
> +	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)
> {

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

[Stanislav Zudin]

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

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

[Konstantin Osipov]

* Stanislav Zudin <szudin@tarantool.org> [19/03/15 22:09]:
> 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));

I'd kindly ask to rewrite this function altogether in a separate
patch.

When pushing data into Lua, we should look at value type,
not column type. AFAICS sql_column_type() is a recent addition.
The name itself -sql_column_is_unsigned() implies we're checking
the property of entire column, not a single value.

Re SQL value types, I would kill sqlite's enum for value types as
well as mem flags and use mp_type wherever possible instead.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[PATCH 03/13] sql: removes unused function.

[Stanislav Zudin]

---
 src/box/sql/main.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 9fe2e2c9d..a3c59b126 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1910,22 +1910,6 @@ sql_uri_boolean(const char *zFilename, const char *zParam, int bDflt)
 	return z ? sqlGetBoolean(z, bDflt) : bDflt;
 }
 
-/*
- * Return a 64-bit integer value for a query parameter.
- */
-sql_int64
-sql_uri_int64(const char *zFilename,	/* Filename as passed to xOpen */
-		  const char *zParam,	/* URI parameter sought */
-		  sql_int64 bDflt)	/* return if parameter is missing */
-{
-	const char *z = sql_uri_parameter(zFilename, zParam);
-	int64_t v;
-	if (z != NULL && sql_dec_or_hex_to_i64(z, false, &v) == 0)
-		bDflt = v;
-	return bDflt;
-}
-
-
 #ifdef SQL_ENABLE_SNAPSHOT
 /*
  * Obtain a snapshot handle for the snapshot of database zDb currently
-- 
2.17.1

[tarantool-patches] Re: [PATCH 03/13] sql: removes unused function.

[n.pettik]

> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index 9fe2e2c9d..a3c59b126 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -1910,22 +1910,6 @@ sql_uri_boolean(const char *zFilename, const char *zParam, int bDflt)
> 	return z ? sqlGetBoolean(z, bDflt) : bDflt;
> }
> 
> -/*
> - * Return a 64-bit integer value for a query parameter.
> - */
> -sql_int64
> -sql_uri_int64(const char *zFilename,	/* Filename as passed to xOpen */
> -		  const char *zParam,	/* URI parameter sought */
> -		  sql_int64 bDflt)	/* return if parameter is missing */
> -{
> -	const char *z = sql_uri_parameter(zFilename, zParam);
> -	int64_t v;
> -	if (z != NULL && sql_dec_or_hex_to_i64(z, false, &v) == 0)
> -		bDflt = v;
> -	return bDflt;
> -}
> -

What about other functions from this family?
They seem to be unused as well.

[tarantool-patches] Re: [PATCH 03/13] sql: removes unused function.

[Stanislav Zudin]

On 25.03.2019 18:11, n.pettik wrote:
> 
>> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
>> index 9fe2e2c9d..a3c59b126 100644
>> --- a/src/box/sql/main.c
>> +++ b/src/box/sql/main.c
>> @@ -1910,22 +1910,6 @@ sql_uri_boolean(const char *zFilename, const char *zParam, int bDflt)
>> 	return z ? sqlGetBoolean(z, bDflt) : bDflt;
>> }
>>
>> -/*
>> - * Return a 64-bit integer value for a query parameter.
>> - */
>> -sql_int64
>> -sql_uri_int64(const char *zFilename,	/* Filename as passed to xOpen */
>> -		  const char *zParam,	/* URI parameter sought */
>> -		  sql_int64 bDflt)	/* return if parameter is missing */
>> -{
>> -	const char *z = sql_uri_parameter(zFilename, zParam);
>> -	int64_t v;
>> -	if (z != NULL && sql_dec_or_hex_to_i64(z, false, &v) == 0)
>> -		bDflt = v;
>> -	return bDflt;
>> -}
>> -
> 
> What about other functions from this family?
> They seem to be unused as well.
> 
> 
Removed unused functions sql_uri_boolean(
and the code underneath SQL_ENABLE_8_3_NAMES.

[PATCH 04/13] sql: support big integers within sql binding

[Stanislav Zudin]

Enables sql binding for unsigned int64.
---
 src/box/execute.c     | 21 ++++++++++----
 src/box/lua/lua_sql.c |  3 ++
 src/box/lua/sql.c     | 10 ++++---
 src/box/sql/func.c    |  7 +++++
 src/box/sql/sqlInt.h  |  4 +++
 src/box/sql/vdbe.c    |  6 ++--
 src/box/sql/vdbeInt.h |  2 +-
 src/box/sql/vdbeapi.c | 66 ++++++++++++++++++++++++++++++++++++++++---
 src/box/sql/vdbemem.c | 12 +++++---
 9 files changed, 109 insertions(+), 22 deletions(-)

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;
-		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);
+		size = mp_sizeof_uint(n);
+		char *pos = (char *) region_alloc(region, size);
+		if (pos == NULL)
+			goto oom;
+		mp_encode_uint(pos, n);
+		break;
+	}
 	case SQL_FLOAT: {
 		double d = sql_column_double(stmt, i);
 		size = mp_sizeof_double(d);
@@ -389,6 +395,9 @@ sql_bind_column(struct sql_stmt *stmt, const struct sql_bind *p,
 	case SQL_INTEGER:
 		rc = sql_bind_int64(stmt, pos, p->i64);
 		break;
+	case SQL_UNSIGNED:
+		rc = sql_bind_uint64(stmt, pos, p->i64);
+		break;
 	case SQL_FLOAT:
 		rc = sql_bind_double(stmt, pos, p->d);
 		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));
+			break;
 		case SQL_FLOAT:
 			lua_pushnumber(L, sql_value_double(param));
 			break;
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
@@ -141,6 +141,7 @@ lengthFunc(sql_context * context, int argc, sql_value ** argv)
 	switch (sql_value_type(argv[0])) {
 	case SQL_BLOB:
 	case SQL_INTEGER:
+	case SQL_UNSIGNED:
 	case SQL_FLOAT:{
 			sql_result_int(context,
 					   sql_value_bytes(argv[0]));
@@ -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]);
+			sql_result_int64(context, iVal);
+			break;
+		}
 	case SQL_NULL:{
 			/* IMP: R-37434-19929 Abs(X) returns NULL if X is NULL. */
 			sql_result_null(context);
@@ -957,6 +963,7 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 					    SQL_TRANSIENT);
 			break;
 		}
+	case SQL_UNSIGNED:
 	case SQL_INTEGER:{
 			sql_result_value(context, argv[0]);
 			break;
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
@@ -644,6 +644,7 @@ enum sql_type {
 	SQL_TEXT = 3,
 	SQL_BLOB = 4,
 	SQL_NULL = 5,
+	SQL_UNSIGNED = 6
 };
 
 /**
@@ -906,6 +907,9 @@ sql_bind_int(sql_stmt *, int, int);
 int
 sql_bind_int64(sql_stmt *, int, sql_int64);
 
+int
+sql_bind_uint64(sql_stmt *, int, sql_uint64);
+
 int
 sql_bind_null(sql_stmt *, int);
 
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);
 	}
 	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 */
 	};
-	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
 sql_result_int(sql_context * pCtx, int iVal)
 {
-	sqlVdbeMemSetInt64(pCtx->pOut, (i64) iVal);
+	sqlVdbeMemSetInt64(pCtx->pOut, (i64) iVal, false);
 }
 
 void
 sql_result_int64(sql_context * pCtx, i64 iVal)
 {
-	sqlVdbeMemSetInt64(pCtx->pOut, iVal);
+	sqlVdbeMemSetInt64(pCtx->pOut, iVal, false);
 }
 
 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");
+		sqlVdbeMemSetInt64(&p->aVar[i - 1], (u64)iValue, true);
 	}
 	return rc;
 }
@@ -1418,6 +1472,10 @@ sql_bind_value(sql_stmt * pStmt, int i, const sql_value * pValue)
 			rc = sql_bind_int64(pStmt, i, pValue->u.i);
 			break;
 		}
+	case SQL_UNSIGNED: {
+			rc = sql_bind_uint64(pStmt, i, pValue->u.i);
+			break;
+		}
 	case SQL_FLOAT:{
 			rc = sql_bind_double(pStmt, i, pValue->u.r);
 			break;
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 0b5ba965e..5e35a1720 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -704,11 +704,13 @@ sqlVdbeMemSetZeroBlob(Mem * pMem, int n)
  * a 64-bit integer.
  */
 static SQL_NOINLINE void
-vdbeReleaseAndSetInt64(Mem * pMem, i64 val)
+vdbeReleaseAndSetInt64(Mem * pMem, i64 val, bool is_unsigned)
 {
 	sqlVdbeMemSetNull(pMem);
 	pMem->u.i = val;
 	pMem->flags = MEM_Int;
+	if (is_unsigned)
+		pMem->flags |= MEM_Unsigned;
 }
 
 /*
@@ -716,13 +718,15 @@ vdbeReleaseAndSetInt64(Mem * pMem, i64 val)
  * manifest type INTEGER.
  */
 void
-sqlVdbeMemSetInt64(Mem * pMem, i64 val)
+sqlVdbeMemSetInt64(Mem * pMem, i64 val, bool is_unsigned)
 {
 	if (VdbeMemDynamic(pMem)) {
-		vdbeReleaseAndSetInt64(pMem, val);
+		vdbeReleaseAndSetInt64(pMem, val, is_unsigned);
 	} else {
 		pMem->u.i = val;
 		pMem->flags = MEM_Int;
+		if (is_unsigned)
+			pMem->flags |= MEM_Unsigned;
 	}
 }
 
@@ -1298,7 +1302,7 @@ valueFromExpr(sql * db,	/* The database connection */
 			goto no_mem;
 		if (ExprHasProperty(pExpr, EP_IntValue)) {
 			sqlVdbeMemSetInt64(pVal,
-					       (i64) pExpr->u.iValue * negInt);
+					       (i64) pExpr->u.iValue * negInt, false);
 		} else {
 			zVal =
 			    sqlMPrintf(db, "%s%s", zNeg, pExpr->u.zToken);
-- 
2.17.1

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

[n.pettik]

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

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

[Stanislav Zudin]

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

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

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/03/25 18:13]:
> > +
> > +		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.

I hope we can kill this array altogether as well as mem flags and
use a combination of mp_type and enum field_type wherever
possible.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Konstantin Osipov]

* Stanislav Zudin <szudin@tarantool.org> [19/03/15 22:09]:
> @@ -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);
> +		size = mp_sizeof_uint(n);
> +		char *pos = (char *) region_alloc(region, size);
> +		if (pos == NULL)
> +			goto oom;
> +		mp_encode_uint(pos, n);
> +		break;
> +	}

Tarantool does have unsigned *field type*, which in future will be
available in SQL as well. I think (ab) using SQL_UNSIGNED type
code which in future will be used for this data type in SQL (or
hopefully we will ditch SQL_* type enum and will use enum
field_type instead in entire SQL) creates a confusion.
Unsigned range is a property of SQL_INTEGER *data type*, not a data
type in itself.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[PATCH 05/13] sql: removes redundant function.

[Stanislav Zudin]

---
 src/box/execute.c     | 2 --
 src/box/lua/sql.c     | 2 --
 src/box/sql/sqlInt.h  | 3 ---
 src/box/sql/vdbeapi.c | 8 --------
 4 files changed, 15 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 31b89a75e..210b9a228 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -241,7 +241,6 @@ 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);
@@ -257,7 +256,6 @@ sql_column_to_messagepack(struct sql_stmt *stmt, int i,
 		break;
 	}
 	case SQL_UNSIGNED: {
-		assert(sql_column_is_unsigned(stmt, i));
 		int64_t n = sql_column_int64(stmt, i);
 		size = mp_sizeof_uint(n);
 		char *pos = (char *) region_alloc(region, size);
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 9a35c03aa..76bb44fa1 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -32,11 +32,9 @@ lua_push_row(struct lua_State *L, struct sql_stmt *stmt)
 		int type = sql_column_type(stmt, i);
 		switch (type) {
 		case SQL_INTEGER:
-			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:
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 56aa7c681..9b1d7df9a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -586,9 +586,6 @@ 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/vdbeapi.c b/src/box/sql/vdbeapi.c
index 2c486552e..c3bdb6f86 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1085,14 +1085,6 @@ sql_column_type(sql_stmt * pStmt, int i)
 	return iType;
 }
 
-bool sql_column_is_unsigned(sql_stmt * pStmt, int i)
-{
-	const struct Mem* pMem = columnMem(pStmt, i);
-	if (!pMem)
-		return false;
-	return (pMem->flags & MEM_Unsigned);
-}
-
 enum sql_subtype
 sql_column_subtype(struct sql_stmt *stmt, int i)
 {
-- 
2.17.1

[tarantool-patches] Re: [PATCH 05/13] sql: removes redundant function.

[n.pettik]

You are removing recently introduced function.
Mb it is worth to avoid using it from the very beginning?

> ---
> src/box/execute.c     | 2 --
> src/box/lua/sql.c     | 2 --
> src/box/sql/sqlInt.h  | 3 ---
> src/box/sql/vdbeapi.c | 8 --------
> 4 files changed, 15 deletions(-)

[PATCH 06/13] sql: aux functions to support big integers

[Stanislav Zudin]

Adapts auxiliary functions for supporting
arithmetic operations with unsigned integers.
---
 src/box/sql/vdbe.c    | 21 ++++++++++++++-------
 src/box/sql/vdbemem.c |  3 +++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8a7f7a12f..ea9d9d98f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -404,15 +404,20 @@ sql_value_apply_type(
  * numeric type, if has one.  Set the pMem->u.r and pMem->u.i fields
  * accordingly.
  */
-static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
+static u32 SQL_NOINLINE computeNumericType(Mem *pMem)
 {
 	assert((pMem->flags & (MEM_Int|MEM_Real))==0);
 	assert((pMem->flags & (MEM_Str|MEM_Blob))!=0);
 	if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0)
 		return 0;
-	if (sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)==ATOI_SIGNED)
-		return MEM_Int;
-	return MEM_Real;
+	switch(sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, pMem->n)) {
+		case ATOI_SIGNED:
+			return MEM_Int;
+		case ATOI_UNSIGNED:
+			return MEM_Int | MEM_Unsigned;
+		default: /* ATOI_OVERFLOW:*/
+			return MEM_Real;
+	}
 }
 
 /*
@@ -422,8 +427,10 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem)
  * Unlike mem_apply_numeric_type(), this routine does not modify pMem->flags.
  * But it does set pMem->u.r and pMem->u.i appropriately.
  */
-static u16 numericType(Mem *pMem)
+static u32 numericType(Mem *pMem)
 {
+	if ((pMem->flags & (MEM_Int|MEM_Unsigned)) == (MEM_Int|MEM_Unsigned))
+		return (MEM_Int|MEM_Unsigned);
 	if (pMem->flags & (MEM_Int|MEM_Real)) {
 		return pMem->flags & (MEM_Int|MEM_Real);
 	}
@@ -1648,8 +1655,8 @@ case OP_Divide:                /* same as TK_SLASH, in1, in2, out3 */
 case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 	char bIntint;   /* Started out as two integer operands */
 	u32 flags;      /* Combined MEM_* flags from both inputs */
-	u16 type1;      /* Numeric type of left operand */
-	u16 type2;      /* Numeric type of right operand */
+	u32 type1;      /* Numeric type of left operand */
+	u32 type2;      /* Numeric type of right operand */
 	i64 iA;         /* Integer value of left operand */
 	i64 iB;         /* Integer value of right operand */
 	double rA;      /* Real value of left operand */
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 5e35a1720..2805d7a01 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -489,6 +489,9 @@ sqlVdbeRealValue(Mem * pMem, double *v)
 	if (pMem->flags & MEM_Real) {
 		*v = pMem->u.r;
 		return 0;
+	} else if (pMem->flags & (MEM_Int | MEM_Unsigned)) {
+		*v = (double)(u64)pMem->u.i;
+		return 0;
 	} else if (pMem->flags & MEM_Int) {
 		*v = (double)pMem->u.i;
 		return 0;
-- 
2.17.1

[tarantool-patches] Re: [PATCH 06/13] sql: aux functions to support big integers

[n.pettik]

Please, squash it with second patch in series.

> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> Adapts auxiliary functions for supporting
> arithmetic operations with unsigned integers.
> ---

[PATCH 07/13] sql: arithmetic functions support big integers

[Stanislav Zudin]

Makes arithmetic functions accept arguments with
values in the range [2^63, 2^64).
---
 src/box/sql/func.c    |   2 +-
 src/box/sql/sqlInt.h  |  23 +++-
 src/box/sql/util.c    | 236 ++++++++++++++++++++++++++++++++----------
 src/box/sql/vdbe.c    |  36 ++++---
 src/box/sql/vdbeInt.h |   2 +-
 5 files changed, 223 insertions(+), 76 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index cf65bf2a2..8a8acc216 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1437,7 +1437,7 @@ sumStep(sql_context * context, int argc, sql_value ** argv)
 			i64 v = sql_value_int64(argv[0]);
 			p->rSum += v;
 			if ((p->approx | p->overflow) == 0
-			    && sqlAddInt64(&p->iSum, v)) {
+			    && sqlAddInt64(&p->iSum, true, v, true) != ATHR_SIGNED) {
 				p->overflow = 1;
 			}
 		} else {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 9b1d7df9a..7f8e3f04e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4383,9 +4383,26 @@ Expr *sqlExprAddCollateString(Parse *, Expr *, const char *);
 Expr *sqlExprSkipCollate(Expr *);
 int sqlCheckIdentifierName(Parse *, char *);
 void sqlVdbeSetChanges(sql *, int);
-int sqlAddInt64(i64 *, i64);
-int sqlSubInt64(i64 *, i64);
-int sqlMulInt64(i64 *, i64);
+
+enum arithmetic_result {
+	/* The result fits the signed 64-bit integer */
+	ATHR_SIGNED,
+	/* The result is positive and fits the
+	 * unsigned 64-bit integer
+	 */
+	ATHR_UNSIGNED,
+	/* The operation causes an overflow */
+	ATHR_OVERFLOW,
+	/* The operation causes division by zero */
+	ATHR_DIVBYZERO
+};
+
+enum arithmetic_result sqlAddInt64(i64 *, bool, i64, bool);
+enum arithmetic_result sqlSubInt64(i64 *, bool, i64, bool);
+enum arithmetic_result sqlMulInt64(i64 *, bool, i64, bool);
+enum arithmetic_result sqlDivInt64(i64 *, bool, i64, bool);
+enum arithmetic_result sqlRemInt64(i64 *, bool, i64, bool);
+
 int sqlAbsInt32(int);
 #ifdef SQL_ENABLE_8_3_NAMES
 void sqlFileSuffix3(const char *, char *);
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index be77f72f8..3786c5083 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -1249,74 +1249,202 @@ sqlSafetyCheckSickOrOk(sql * db)
 }
 
 /*
- * Attempt to add, substract, or multiply the 64-bit signed value iB against
- * the other 64-bit signed integer at *pA and store the result in *pA.
- * Return 0 on success.  Or if the operation would have resulted in an
- * overflow, leave *pA unchanged and return 1.
+ * Get modulo of 64-bit number
  */
-int
-sqlAddInt64(i64 * pA, i64 iB)
+static u64 mod64(i64 v, bool is_signed)
+{
+	bool is_neg = v < 0 && is_signed;
+	if (is_neg)
+		return (v == INT64_MIN) ? (u64)v : (u64)(-v);
+	else
+		return (u64)v;
+}
+
+/*
+ * Attempt to add, substract, or multiply the 64-bit value iB against
+ * the other 64-bit integer at *pA and store the result in *pA.
+ * Return ATHR_SIGNED or ATHR_UNSIGNED on success.
+ * Or if the operation would have resulted in an
+ * overflow, leave *pA unchanged and return ATHR_OVERFLOW.
+ */
+enum arithmetic_result
+sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
 {
 	i64 iA = *pA;
-	testcase(iA == 0);
-	testcase(iA == 1);
-	testcase(iB == -1);
-	testcase(iB == 0);
-	if (iB >= 0) {
-		testcase(iA > 0 && LARGEST_INT64 - iA == iB);
-		testcase(iA > 0 && LARGEST_INT64 - iA == iB - 1);
-		if (iA > 0 && LARGEST_INT64 - iA < iB)
-			return 1;
+
+	bool is_negA = iA < 0 && is_signedA;
+	bool is_negB = iB < 0 && is_signedB;
+
+	/* Make sure we've got only one combination of
+	 * positive and negative operands
+	 */
+	if (is_negA > is_negB) {
+		SWAP(is_negA, is_negB);
+		SWAP(iA, iB);
+	}
+
+	if (is_negA != is_negB) {
+
+		assert(iA >=0 && iB < 0);
+		u64 uB = mod64(iB, true);
+
+		if ((u64)iA >= uB) {
+			u64 sum = (u64)iA - uB;
+			*pA = (i64)sum;
+			return (sum <= INT64_MAX) ? ATHR_SIGNED
+						  : ATHR_UNSIGNED;
+		} else {
+			u64 sum = uB - (u64)iA;
+			if (sum == INT64_MIN_MOD) {
+				*pA = INT64_MIN;
+			} else {
+				assert(sum < INT64_MAX);
+				*pA = -(i64)sum;
+			}
+			return ATHR_SIGNED;
+		}
+	}
+
+	if (is_negA) {
+		assert(is_signedA && is_signedB);
+		if (-(iA + LARGEST_INT64) > iB + 1)
+			return ATHR_OVERFLOW;
+		*pA = iA + iB;
+		return ATHR_SIGNED;
 	} else {
-		testcase(iA < 0 && -(iA + LARGEST_INT64) == iB + 1);
-		testcase(iA < 0 && -(iA + LARGEST_INT64) == iB + 2);
-		if (iA < 0 && -(iA + LARGEST_INT64) > iB + 1)
-			return 1;
+		if (UINT64_MAX - (u64)iA < (u64)iB)
+			return ATHR_OVERFLOW;
+
+		u64 sum = (u64)iA + (u64)iB;
+		*pA = (i64)sum;
+		return (sum <= INT64_MAX) ? ATHR_SIGNED
+					  : ATHR_UNSIGNED;
 	}
-	*pA += iB;
-	return 0;
 }
 
-int
-sqlSubInt64(i64 * pA, i64 iB)
+enum arithmetic_result
+sqlSubInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
 {
-	testcase(iB == SMALLEST_INT64 + 1);
-	if (iB == SMALLEST_INT64) {
-		testcase((*pA) == (-1));
-		testcase((*pA) == 0);
-		if ((*pA) >= 0)
-			return 1;
-		*pA -= iB;
-		return 0;
+	i64 iA = *pA;
+
+	bool is_negA = iA < 0 && is_signedA;
+	bool is_negB = iB < 0 && is_signedB;
+
+	if (is_negA) {
+		if (!is_signedB){
+			assert((u64)iB > INT64_MAX);
+			return ATHR_OVERFLOW;
+		}
+
+		if (iB == INT64_MIN)
+			return ATHR_OVERFLOW;
+		else
+			return sqlAddInt64(pA, true, -iB, true);
+	}
+
+	if (is_negB) {
+		/* iA - (-iB) => iA + iB */
+		u64 uB = mod64(iB, true);
+		if (iB == INT64_MIN)
+			is_signedB = false;
+
+		return sqlAddInt64(pA, is_signedA, uB, is_signedB);
 	} else {
-		return sqlAddInt64(pA, -iB);
+		/* Both iA & iB are positive */
+		if ((u64)iA < (u64)iB)
+			return  ATHR_OVERFLOW;
+		u64 val = (u64)iA - (u64)iB;
+		*pA = (i64)val;
+		return (val > INT64_MAX) ? ATHR_UNSIGNED
+					 : ATHR_SIGNED;
 	}
 }
 
-int
-sqlMulInt64(i64 * pA, i64 iB)
+static enum arithmetic_result
+apply_sign(i64* pOut, u64 value, bool is_neg)
 {
-	i64 iA = *pA;
-	if (iB > 0) {
-		if (iA > LARGEST_INT64 / iB)
-			return 1;
-		if (iA < SMALLEST_INT64 / iB)
-			return 1;
-	} else if (iB < 0) {
-		if (iA > 0) {
-			if (iB < SMALLEST_INT64 / iA)
-				return 1;
-		} else if (iA < 0) {
-			if (iB == SMALLEST_INT64)
-				return 1;
-			if (iA == SMALLEST_INT64)
-				return 1;
-			if (-iA > LARGEST_INT64 / -iB)
-				return 1;
-		}
+	if (is_neg) {
+		if (value > INT64_MIN_MOD)
+			return ATHR_OVERFLOW;
+		else if (value == INT64_MIN_MOD)
+			*pOut = (i64)value;
+		else
+			*pOut = -(i64)value;
+
+		return ATHR_SIGNED;
 	}
-	*pA = iA * iB;
-	return 0;
+
+	*pOut = (i64) value;
+	return (value > INT64_MAX) ? ATHR_UNSIGNED
+			  	   : ATHR_SIGNED;
+}
+
+enum arithmetic_result
+sqlMulInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
+{
+	if (*pA == 0 || iB == 0) {
+		*pA = 0;
+		return ATHR_SIGNED;
+	}
+
+	bool is_negA = *pA < 0 && is_signedA;
+	bool is_negB = iB < 0 && is_signedB;
+
+	bool is_neg = is_negA != is_negB;
+
+	u64 uA = mod64(*pA, is_signedA);
+	u64 uB = mod64(iB, is_signedB);
+
+	if (is_neg) {
+		if (INT64_MIN_MOD / uA < uB)
+			return ATHR_OVERFLOW;
+	} else {
+		if (INT64_MAX / uA < uB)
+			return ATHR_OVERFLOW;
+	}
+
+	u64 mul = uA * uB;
+	return apply_sign(pA, mul, is_neg);
+}
+
+enum arithmetic_result
+sqlDivInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB) {
+	if (*pA == 0)
+		return ATHR_SIGNED;
+	if (iB == 0)
+		return ATHR_DIVBYZERO;
+
+	bool is_negA = *pA < 0 && is_signedA;
+	bool is_negB = iB < 0 && is_signedB;
+
+	bool is_neg = is_negA != is_negB;
+
+	u64 uA = mod64(*pA, is_signedA);
+	u64 uB = mod64(iB, is_signedB);
+
+	u64 div = uA / uB;
+	return apply_sign(pA, div, is_neg);
+}
+
+enum arithmetic_result
+sqlRemInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB) {
+
+	if (iB == 0)
+		return ATHR_DIVBYZERO;
+	/*
+	 * The sign of the remainder is defined in such
+	 * a way that if the quotient a/b is representable
+	 * in the result type, then (a/b)*b + a%b == a.
+	 *
+	 * The 2nd operand doesn't affect the sign of result.
+	 */
+
+	bool is_neg = *pA < 0 && is_signedA;
+	u64 uA = mod64(*pA, is_signedA);
+	u64 uB = mod64(iB, is_signedB);
+
+	u64 rem = uA % uB;
+	return apply_sign(pA, rem, is_neg);
 }
 
 /*
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ea9d9d98f..d4bd845fb 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1672,28 +1672,29 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 	if ((type1 & type2 & MEM_Int)!=0) {
 		iA = pIn1->u.i;
 		iB = pIn2->u.i;
+		bool is_signedA = (type1 & MEM_Unsigned) == 0;
+		bool is_signedB = (type2 & MEM_Unsigned) == 0;
 		bIntint = 1;
+		enum arithmetic_result arr;
 		switch( pOp->opcode) {
-		case OP_Add:       if (sqlAddInt64(&iB,iA)) goto integer_overflow; break;
-		case OP_Subtract:  if (sqlSubInt64(&iB,iA)) goto integer_overflow; break;
-		case OP_Multiply:  if (sqlMulInt64(&iB,iA)) goto integer_overflow; break;
-		case OP_Divide: {
-			if (iA == 0)
-				goto division_by_zero;
-			if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow;
-			iB /= iA;
-			break;
+		case OP_Add:       arr = sqlAddInt64(&iB, is_signedA, iA, is_signedB); break;
+		case OP_Subtract:  arr = sqlSubInt64(&iB, is_signedA, iA, is_signedB); break;
+		case OP_Multiply:  arr = sqlMulInt64(&iB, is_signedA, iA, is_signedB); break;
+		case OP_Divide:    arr = sqlDivInt64(&iB, is_signedA, iA, is_signedB); break;
+		default: 	   arr = sqlRemInt64(&iB, is_signedA, iA, is_signedB); break;
 		}
-		default: {
-			if (iA == 0)
-				goto division_by_zero;
-			if (iA==-1) iA = 1;
-			iB %= iA;
+
+		switch(arr){
+		case ATHR_SIGNED:
+			MemSetTypeFlag(pOut, MEM_Int);
 			break;
-		}
+		case ATHR_UNSIGNED:
+			MemSetTypeFlag(pOut, MEM_Int|MEM_Unsigned);
+			break;
+		case ATHR_OVERFLOW:	goto integer_overflow;
+		case ATHR_DIVBYZERO:	goto division_by_zero;
 		}
 		pOut->u.i = iB;
-		MemSetTypeFlag(pOut, MEM_Int);
 	} else {
 		bIntint = 0;
 		if (sqlVdbeRealValue(pIn1, &rA) != 0) {
@@ -5177,7 +5178,8 @@ case OP_OffsetLimit: {    /* in1, out2, in3 */
 	assert(pIn1->flags & MEM_Int);
 	assert(pIn3->flags & MEM_Int);
 	x = pIn1->u.i;
-	if (x<=0 || sqlAddInt64(&x, pIn3->u.i>0?pIn3->u.i:0)) {
+	if (x<=0 ||
+	    sqlAddInt64(&x, true, pIn3->u.i>0?pIn3->u.i:0, true) != ATHR_SIGNED) {
 		/* If the LIMIT is less than or equal to zero, loop forever.  This
 		 * is documented.  But also, if the LIMIT+OFFSET exceeds 2^63 then
 		 * also loop forever.  This is undocumented.  In fact, one could argue
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 0375845d9..42f22df52 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -276,7 +276,7 @@ enum {
  * Clear any existing type flags from a Mem and replace them with f
  */
 #define MemSetTypeFlag(p, f) \
-   ((p)->flags = ((p)->flags&~(MEM_TypeMask|MEM_Zero))|f)
+   ((p)->flags = ((p)->flags&~(MEM_TypeMask|MEM_Zero|MEM_Unsigned))|f)
 
 /*
  * Return true if a memory cell is not marked as invalid.  This macro
-- 
2.17.1

[tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers

[n.pettik]

> Makes arithmetic functions accept arguments with
> values in the range [2^63, 2^64).
> ---
> src/box/sql/func.c    |   2 +-
> src/box/sql/sqlInt.h  |  23 +++-
> src/box/sql/util.c    | 236 ++++++++++++++++++++++++++++++++----------
> src/box/sql/vdbe.c    |  36 ++++---
> src/box/sql/vdbeInt.h |   2 +-
> 5 files changed, 223 insertions(+), 76 deletions(-)
> 
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 9b1d7df9a..7f8e3f04e 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4383,9 +4383,26 @@ Expr *sqlExprAddCollateString(Parse *, Expr *, const char *);
> Expr *sqlExprSkipCollate(Expr *);
> int sqlCheckIdentifierName(Parse *, char *);
> void sqlVdbeSetChanges(sql *, int);
> -int sqlAddInt64(i64 *, i64);
> -int sqlSubInt64(i64 *, i64);
> -int sqlMulInt64(i64 *, i64);
> +
> +enum arithmetic_result {
> +	/* The result fits the signed 64-bit integer */
> +	ATHR_SIGNED,
> +	/* The result is positive and fits the
> +	 * unsigned 64-bit integer
> +	 */
> +	ATHR_UNSIGNED,
> +	/* The operation causes an overflow */
> +	ATHR_OVERFLOW,
> +	/* The operation causes division by zero */
> +	ATHR_DIVBYZERO
> +};
> +
> +enum arithmetic_result sqlAddInt64(i64 *, bool, i64, bool);
> +enum arithmetic_result sqlSubInt64(i64 *, bool, i64, bool);
> +enum arithmetic_result sqlMulInt64(i64 *, bool, i64, bool);
> +enum arithmetic_result sqlDivInt64(i64 *, bool, i64, bool);
> +enum arithmetic_result sqlRemInt64(i64 *, bool, i64, bool);

Since you’ve already fixed signature of these functions,
please make them follow Tarantool code style:

enum arithmetic_result
sql_add_int64(int64_t *lhs, bool is_lhs_signed, …);

What is more, personally I would apply the same fix as for atoi functions:
make them return -1 in case of overflow or division by 0 and set
diag message right in these functions; use enum to represent their args.

> +
> int sqlAbsInt32(int);
> #ifdef SQL_ENABLE_8_3_NAMES
> void sqlFileSuffix3(const char *, char *);
> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index be77f72f8..3786c5083 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -1249,74 +1249,202 @@ sqlSafetyCheckSickOrOk(sql * db)
> }
> 
> +/*
> + * Attempt to add, substract, or multiply the 64-bit value iB against
> + * the other 64-bit integer at *pA and store the result in *pA.
> + * Return ATHR_SIGNED or ATHR_UNSIGNED on success.
> + * Or if the operation would have resulted in an
> + * overflow, leave *pA unchanged and return ATHR_OVERFLOW.
> + */
> +enum arithmetic_result
> +sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
> {
> 	i64 iA = *pA;
> -	testcase(iA == 0);
> -	testcase(iA == 1);
> -	testcase(iB == -1);
> -	testcase(iB == 0);
> -	if (iB >= 0) {
> -		testcase(iA > 0 && LARGEST_INT64 - iA == iB);
> -		testcase(iA > 0 && LARGEST_INT64 - iA == iB - 1);
> -		if (iA > 0 && LARGEST_INT64 - iA < iB)
> -			return 1;
> +
> +	bool is_negA = iA < 0 && is_signedA;
> +	bool is_negB = iB < 0 && is_signedB;
> +
> +	/* Make sure we've got only one combination of
> +	 * positive and negative operands
> +	 */

Nit: note that correct way of comment formatting is:

/*
 * Make sure we've got only one combination of
 * positive and negative operands.
 */

> /*
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index ea9d9d98f..d4bd845fb 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1672,28 +1672,29 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
> 	if ((type1 & type2 & MEM_Int)!=0) {
> 		iA = pIn1->u.i;
> 		iB = pIn2->u.i;
> +		bool is_signedA = (type1 & MEM_Unsigned) == 0;
> +		bool is_signedB = (type2 & MEM_Unsigned) == 0;
> 		bIntint = 1;
> +		enum arithmetic_result arr;
> 		switch( pOp->opcode) {
> -		case OP_Add:       if (sqlAddInt64(&iB,iA)) goto integer_overflow; break;
> -		case OP_Subtract:  if (sqlSubInt64(&iB,iA)) goto integer_overflow; break;
> -		case OP_Multiply:  if (sqlMulInt64(&iB,iA)) goto integer_overflow; break;
> -		case OP_Divide: {
> -			if (iA == 0)
> -				goto division_by_zero;
> -			if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow;
> -			iB /= iA;
> -			break;
> +		case OP_Add:       arr = sqlAddInt64(&iB, is_signedA, iA, is_signedB); break;
> +		case OP_Subtract:  arr = sqlSubInt64(&iB, is_signedA, iA, is_signedB); break;
> +		case OP_Multiply:  arr = sqlMulInt64(&iB, is_signedA, iA, is_signedB); break;
> +		case OP_Divide:    arr = sqlDivInt64(&iB, is_signedA, iA, is_signedB); break;
> +		default: 	   arr = sqlRemInt64(&iB, is_signedA, iA, is_signedB); break;

SQL ANSI specifications doesn’t provide description of unsigned behaviour.
But for example in C there is no unsigned overflow, because if result can’t
be represented by unsigned range, it is truncated to modulo (MAX_UINT + 1 == 1).
Should we follow this way? IDK, it needs discussion involving other team members.

[tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers

[Stanislav Zudin]

On 25.03.2019 18:13, n.pettik wrote:
> 
>> Makes arithmetic functions accept arguments with
>> values in the range [2^63, 2^64).
>> ---
>> src/box/sql/func.c    |   2 +-
>> src/box/sql/sqlInt.h  |  23 +++-
>> src/box/sql/util.c    | 236 ++++++++++++++++++++++++++++++++----------
>> src/box/sql/vdbe.c    |  36 ++++---
>> src/box/sql/vdbeInt.h |   2 +-
>> 5 files changed, 223 insertions(+), 76 deletions(-)
>>
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index 9b1d7df9a..7f8e3f04e 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -4383,9 +4383,26 @@ Expr *sqlExprAddCollateString(Parse *, Expr *, const char *);
>> Expr *sqlExprSkipCollate(Expr *);
>> int sqlCheckIdentifierName(Parse *, char *);
>> void sqlVdbeSetChanges(sql *, int);
>> -int sqlAddInt64(i64 *, i64);
>> -int sqlSubInt64(i64 *, i64);
>> -int sqlMulInt64(i64 *, i64);
>> +
>> +enum arithmetic_result {
>> +	/* The result fits the signed 64-bit integer */
>> +	ATHR_SIGNED,
>> +	/* The result is positive and fits the
>> +	 * unsigned 64-bit integer
>> +	 */
>> +	ATHR_UNSIGNED,
>> +	/* The operation causes an overflow */
>> +	ATHR_OVERFLOW,
>> +	/* The operation causes division by zero */
>> +	ATHR_DIVBYZERO
>> +};
>> +
>> +enum arithmetic_result sqlAddInt64(i64 *, bool, i64, bool);
>> +enum arithmetic_result sqlSubInt64(i64 *, bool, i64, bool);
>> +enum arithmetic_result sqlMulInt64(i64 *, bool, i64, bool);
>> +enum arithmetic_result sqlDivInt64(i64 *, bool, i64, bool);
>> +enum arithmetic_result sqlRemInt64(i64 *, bool, i64, bool);
> 
> Since you’ve already fixed signature of these functions,
> please make them follow Tarantool code style:
> 
> enum arithmetic_result
> sql_add_int64(int64_t *lhs, bool is_lhs_signed, …);

Done.

> 
> What is more, personally I would apply the same fix as for atoi functions:
> make them return -1 in case of overflow or division by 0 and set
> diag message right in these functions; use enum to represent their args.

It's a bad practice to write diagnostic from the low-level functions.
Using a single enum as a return value gives a compact readable code.

> 
>> +
>> int sqlAbsInt32(int);
>> #ifdef SQL_ENABLE_8_3_NAMES
>> void sqlFileSuffix3(const char *, char *);
>> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
>> index be77f72f8..3786c5083 100644
>> --- a/src/box/sql/util.c
>> +++ b/src/box/sql/util.c
>> @@ -1249,74 +1249,202 @@ sqlSafetyCheckSickOrOk(sql * db)
>> }
>>
>> +/*
>> + * Attempt to add, substract, or multiply the 64-bit value iB against
>> + * the other 64-bit integer at *pA and store the result in *pA.
>> + * Return ATHR_SIGNED or ATHR_UNSIGNED on success.
>> + * Or if the operation would have resulted in an
>> + * overflow, leave *pA unchanged and return ATHR_OVERFLOW.
>> + */
>> +enum arithmetic_result
>> +sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
>> {
>> 	i64 iA = *pA;
>> -	testcase(iA == 0);
>> -	testcase(iA == 1);
>> -	testcase(iB == -1);
>> -	testcase(iB == 0);
>> -	if (iB >= 0) {
>> -		testcase(iA > 0 && LARGEST_INT64 - iA == iB);
>> -		testcase(iA > 0 && LARGEST_INT64 - iA == iB - 1);
>> -		if (iA > 0 && LARGEST_INT64 - iA < iB)
>> -			return 1;
>> +
>> +	bool is_negA = iA < 0 && is_signedA;
>> +	bool is_negB = iB < 0 && is_signedB;
>> +
>> +	/* Make sure we've got only one combination of
>> +	 * positive and negative operands
>> +	 */
> 
> Nit: note that correct way of comment formatting is:
> 
> /*
>   * Make sure we've got only one combination of
>   * positive and negative operands.
>   */
> 

Fixed.

>> /*
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index ea9d9d98f..d4bd845fb 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -1672,28 +1672,29 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
>> 	if ((type1 & type2 & MEM_Int)!=0) {
>> 		iA = pIn1->u.i;
>> 		iB = pIn2->u.i;
>> +		bool is_signedA = (type1 & MEM_Unsigned) == 0;
>> +		bool is_signedB = (type2 & MEM_Unsigned) == 0;
>> 		bIntint = 1;
>> +		enum arithmetic_result arr;
>> 		switch( pOp->opcode) {
>> -		case OP_Add:       if (sqlAddInt64(&iB,iA)) goto integer_overflow; break;
>> -		case OP_Subtract:  if (sqlSubInt64(&iB,iA)) goto integer_overflow; break;
>> -		case OP_Multiply:  if (sqlMulInt64(&iB,iA)) goto integer_overflow; break;
>> -		case OP_Divide: {
>> -			if (iA == 0)
>> -				goto division_by_zero;
>> -			if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow;
>> -			iB /= iA;
>> -			break;
>> +		case OP_Add:       arr = sqlAddInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		case OP_Subtract:  arr = sqlSubInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		case OP_Multiply:  arr = sqlMulInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		case OP_Divide:    arr = sqlDivInt64(&iB, is_signedA, iA, is_signedB); break;
>> +		default: 	   arr = sqlRemInt64(&iB, is_signedA, iA, is_signedB); break;
> 
> SQL ANSI specifications doesn’t provide description of unsigned behaviour.
> But for example in C there is no unsigned overflow, because if result can’t
> be represented by unsigned range, it is truncated to modulo (MAX_UINT + 1 == 1).
> Should we follow this way? IDK, it needs discussion involving other team members.

Good point.

> 
> 

[tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers

[Konstantin Osipov]

* Stanislav Zudin <szudin@tarantool.org> [19/04/01 23:44]:
> > What is more, personally I would apply the same fix as for atoi functions:
> > make them return -1 in case of overflow or division by 0 and set
> > diag message right in these functions; use enum to represent their args.
> 
> It's a bad practice to write diagnostic from the low-level functions.
> Using a single enum as a return value gives a compact readable code.

I'd agree with this if I knew there could be some context where we
would use these functions without needing to set the diagnostics
area. I can not imagine such a case. So I'd make things simple
right now and assume it's a simple to-do refactoring should we
need it in the future.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/03/25 18:13]:
> > +enum arithmetic_result sqlAddInt64(i64 *, bool, i64, bool);
> > +enum arithmetic_result sqlSubInt64(i64 *, bool, i64, bool);
> > +enum arithmetic_result sqlMulInt64(i64 *, bool, i64, bool);
> > +enum arithmetic_result sqlDivInt64(i64 *, bool, i64, bool);
> > +enum arithmetic_result sqlRemInt64(i64 *, bool, i64, bool);

I think we should not introduce a separate enum for this, the
convention should be:

-1 + diag_set for errors

- the argument list should be designed in a way to make the
  functions composable. E.g. if you input bool is_signed as input,
  it should return bool is_signed for output. 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[PATCH 08/13] sql: aggregate sql functions support big int

[Stanislav Zudin]

Makes sql functions avg() and sum() accept arguments with
values in the range [2^63, 2^64).
---
 src/box/sql/func.c    | 35 ++++++++++++++++++++++++++++++-----
 src/box/sql/sqlInt.h  |  3 +++
 src/box/sql/vdbeapi.c |  6 ++++++
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 8a8acc216..194dec252 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1410,6 +1410,7 @@ struct SumCtx {
 	i64 cnt;		/* Number of elements summed */
 	u8 overflow;		/* True if integer overflow seen */
 	u8 approx;		/* True if non-integer value was input to the sum */
+	u8 is_unsigned;		/* True if value exceeded 2^63 */
 };
 
 /*
@@ -1433,16 +1434,38 @@ sumStep(sql_context * context, int argc, sql_value ** argv)
 	type = sql_value_numeric_type(argv[0]);
 	if (p && type != SQL_NULL) {
 		p->cnt++;
+		i64 v = 0;
+		bool is_signed = false;
+
 		if (type == SQL_INTEGER) {
-			i64 v = sql_value_int64(argv[0]);
+			v = sql_value_int64(argv[0]);
 			p->rSum += v;
-			if ((p->approx | p->overflow) == 0
-			    && sqlAddInt64(&p->iSum, true, v, true) != ATHR_SIGNED) {
-				p->overflow = 1;
-			}
+			is_signed = true;
+		} else if (type == SQL_UNSIGNED) {
+			v = sql_value_int64(argv[0]);
+			p->rSum += (u64)v;
+			is_signed = false;
 		} else {
 			p->rSum += sql_value_double(argv[0]);
 			p->approx = 1;
+			return;
+		}
+
+		/* proceed with the integer value */
+		if ((p->approx | p->overflow) == 0) {
+			enum arithmetic_result r = sqlAddInt64(&p->iSum,
+							       p->is_unsigned == 0,
+							       v, is_signed);
+			switch (r) {
+			case ATHR_SIGNED:
+				break;
+			case ATHR_UNSIGNED:
+				p->is_unsigned = 1;
+				break;
+			default:
+				p->overflow = 1;
+				break;
+			}
 		}
 	}
 }
@@ -1455,6 +1478,8 @@ sumFinalize(sql_context * context)
 	if (p && p->cnt > 0) {
 		if (p->overflow) {
 			sql_result_error(context, "integer overflow", -1);
+		} else if (p->is_unsigned) {
+			sql_result_uint64(context, (u64)p->iSum);
 		} else if (p->approx) {
 			sql_result_double(context, p->rSum);
 		} else {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 7f8e3f04e..c43d8c193 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -495,6 +495,9 @@ sql_result_int(sql_context *, int);
 void
 sql_result_int64(sql_context *, sql_int64);
 
+void
+sql_result_uint64(sql_context *, sql_uint64);
+
 void
 sql_result_null(sql_context *);
 
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index c3bdb6f86..6a3413954 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -451,6 +451,12 @@ sql_result_int64(sql_context * pCtx, i64 iVal)
 	sqlVdbeMemSetInt64(pCtx->pOut, iVal, false);
 }
 
+void
+sql_result_uint64(sql_context * pCtx, u64 iVal)
+{
+	sqlVdbeMemSetInt64(pCtx->pOut, iVal, iVal > INT64_MAX);
+}
+
 void
 sql_result_null(sql_context * pCtx)
 {
-- 
2.17.1

[tarantool-patches] Re: [PATCH 08/13] sql: aggregate sql functions support big int

[n.pettik]

> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> Makes sql functions avg() and sum() accept arguments with
> values in the range [2^63, 2^64).
> ---
> src/box/sql/func.c    | 35 ++++++++++++++++++++++++++++++-----
> src/box/sql/sqlInt.h  |  3 +++
> src/box/sql/vdbeapi.c |  6 ++++++
> 3 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 8a8acc216..194dec252 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1410,6 +1410,7 @@ struct SumCtx {
> 	i64 cnt;		/* Number of elements summed */
> 	u8 overflow;		/* True if integer overflow seen */
> 	u8 approx;		/* True if non-integer value was input to the sum */
> +	u8 is_unsigned;		/* True if value exceeded 2^63 */

Bool is enough.

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 7f8e3f04e..c43d8c193 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -495,6 +495,9 @@ sql_result_int(sql_context *, int);
> void
> sql_result_int64(sql_context *, sql_int64);
> 
> +void
> +sql_result_uint64(sql_context *, sql_uint64);
> +

Add test cases verifying that aggregate functions can handle uints.

[tarantool-patches] Re: [PATCH 08/13] sql: aggregate sql functions support big int

[Stanislav Zudin]

On 25.03.2019 18:13, n.pettik wrote:
> 
> 
>> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
>>
>> Makes sql functions avg() and sum() accept arguments with
>> values in the range [2^63, 2^64).
>> ---
>> src/box/sql/func.c    | 35 ++++++++++++++++++++++++++++++-----
>> src/box/sql/sqlInt.h  |  3 +++
>> src/box/sql/vdbeapi.c |  6 ++++++
>> 3 files changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index 8a8acc216..194dec252 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -1410,6 +1410,7 @@ struct SumCtx {
>> 	i64 cnt;		/* Number of elements summed */
>> 	u8 overflow;		/* True if integer overflow seen */
>> 	u8 approx;		/* True if non-integer value was input to the sum */
>> +	u8 is_unsigned;		/* True if value exceeded 2^63 */
> 
> Bool is enough.

Done.

> 
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index 7f8e3f04e..c43d8c193 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -495,6 +495,9 @@ sql_result_int(sql_context *, int);
>> void
>> sql_result_int64(sql_context *, sql_int64);
>>
>> +void
>> +sql_result_uint64(sql_context *, sql_uint64);
>> +
> 
> Add test cases verifying that aggregate functions can handle uints.
> 
Done
> 
> 

Re: [tarantool-patches] [PATCH 08/13] sql: aggregate sql functions support big int

[Konstantin Osipov]

* Stanislav Zudin <szudin@tarantool.org> [19/03/15 22:09]:
>  		if (type == SQL_INTEGER) {
> -			i64 v = sql_value_int64(argv[0]);
> +			v = sql_value_int64(argv[0]);
>  			p->rSum += v;
> -			if ((p->approx | p->overflow) == 0
> -			    && sqlAddInt64(&p->iSum, true, v, true) != ATHR_SIGNED) {
> -				p->overflow = 1;
> -			}
> +			is_signed = true;
> +		} else if (type == SQL_UNSIGNED) {
> +			v = sql_value_int64(argv[0]);
> +			p->rSum += (u64)v;
> +			is_signed = false;

Perhaps I am missing something in this patch, but where did
overflow check go? How does this work if p->rSum contains a
negative value?


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[PATCH 09/13] sql: fixes errors

[Stanislav Zudin]

Fixes an error in sql_value_type()
and wrong arguments order in arithmetic operations.
---
 src/box/sql/vdbe.c    | 10 +++++-----
 src/box/sql/vdbeapi.c |  3 ++-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d4bd845fb..ad2ce1787 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1677,11 +1677,11 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 		bIntint = 1;
 		enum arithmetic_result arr;
 		switch( pOp->opcode) {
-		case OP_Add:       arr = sqlAddInt64(&iB, is_signedA, iA, is_signedB); break;
-		case OP_Subtract:  arr = sqlSubInt64(&iB, is_signedA, iA, is_signedB); break;
-		case OP_Multiply:  arr = sqlMulInt64(&iB, is_signedA, iA, is_signedB); break;
-		case OP_Divide:    arr = sqlDivInt64(&iB, is_signedA, iA, is_signedB); break;
-		default: 	   arr = sqlRemInt64(&iB, is_signedA, iA, is_signedB); break;
+		case OP_Add:       arr = sqlAddInt64(&iB, is_signedB, iA, is_signedA); break;
+		case OP_Subtract:  arr = sqlSubInt64(&iB, is_signedB, iA, is_signedA); break;
+		case OP_Multiply:  arr = sqlMulInt64(&iB, is_signedB, iA, is_signedA); break;
+		case OP_Divide:    arr = sqlDivInt64(&iB, is_signedB, iA, is_signedA); break;
+		default: 	   arr = sqlRemInt64(&iB, is_signedB, iA, is_signedA); break;
 		}
 
 		switch(arr){
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 6a3413954..06140569c 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -319,7 +319,8 @@ sql_value_type(sql_value * pVal)
 	 * type bits, to make them applicable for
 	 * array indexing.
 	 */
-	u32 offset = (pVal->flags >> 12) | (pVal->flags & MEM_PURE_TYPE_MASK);
+	u32 offset = ((pVal->flags & MEM_Unsigned) >> 12) |
+		     (pVal->flags & MEM_PURE_TYPE_MASK);
 	assert(offset < 0x40);
 	return aType[offset];
 }
-- 
2.17.1

[tarantool-patches] Re: [PATCH 09/13] sql: fixes errors

[n.pettik]

> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> Fixes an error in sql_value_type()
> and wrong arguments order in arithmetic operations.

You are fixing errors made in previous patches. Please, merge this
patch with corresponding one.

[PATCH 10/13] sql: fixes an error in sqlSubInt64

[Stanislav Zudin]

---
 src/box/sql/util.c | 54 +++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 3786c5083..980932f3b 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -1260,6 +1260,25 @@ static u64 mod64(i64 v, bool is_signed)
 		return (u64)v;
 }
 
+static enum arithmetic_result
+apply_sign(i64* pOut, u64 value, bool is_neg)
+{
+	if (is_neg) {
+		if (value > INT64_MIN_MOD)
+			return ATHR_OVERFLOW;
+		else if (value == INT64_MIN_MOD)
+			*pOut = (i64)value;
+		else
+			*pOut = -(i64)value;
+
+		return ATHR_SIGNED;
+	}
+
+	*pOut = (i64) value;
+	return (value > INT64_MAX) ? ATHR_UNSIGNED
+				   : ATHR_SIGNED;
+}
+
 /*
  * Attempt to add, substract, or multiply the 64-bit value iB against
  * the other 64-bit integer at *pA and store the result in *pA.
@@ -1351,32 +1370,17 @@ sqlSubInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
 		return sqlAddInt64(pA, is_signedA, uB, is_signedB);
 	} else {
 		/* Both iA & iB are positive */
-		if ((u64)iA < (u64)iB)
-			return  ATHR_OVERFLOW;
-		u64 val = (u64)iA - (u64)iB;
-		*pA = (i64)val;
-		return (val > INT64_MAX) ? ATHR_UNSIGNED
-					 : ATHR_SIGNED;
-	}
-}
-
-static enum arithmetic_result
-apply_sign(i64* pOut, u64 value, bool is_neg)
-{
-	if (is_neg) {
-		if (value > INT64_MIN_MOD)
-			return ATHR_OVERFLOW;
-		else if (value == INT64_MIN_MOD)
-			*pOut = (i64)value;
-		else
-			*pOut = -(i64)value;
-
-		return ATHR_SIGNED;
+		if ((u64)iA < (u64)iB) {
+			/* subtract with sign changing */
+			u64 val = (u64)iB - (u64)iA;
+			return apply_sign(pA, val, true);
+		} else {
+			u64 val = (u64)iA - (u64)iB;
+			*pA = (i64)val;
+			return (val > INT64_MAX) ? ATHR_UNSIGNED
+						 : ATHR_SIGNED;
+		}
 	}
-
-	*pOut = (i64) value;
-	return (value > INT64_MAX) ? ATHR_UNSIGNED
-			  	   : ATHR_SIGNED;
 }
 
 enum arithmetic_result
-- 
2.17.1

[tarantool-patches] Re: [PATCH 10/13] sql: fixes an error in sqlSubInt64

[n.pettik]

The same: squash this patch. 

[PATCH 11/13] sql: fixes an error in string to int64 conversion

[Stanislav Zudin]

Returns an error if length of the recognized 
string is less than length of the token.
---
 src/box/sql/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 980932f3b..17268aaaa 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -599,6 +599,7 @@ sqlAtoF(const char *z, double *pResult, int length)
 enum atoi_result
 sql_atoi64(const char *z, int64_t *val, int length)
 {
+	const char* expected_end = z + length;
 	int neg = 0;		/* assume positive */
 	const char *zEnd = z + length;
 	int incr = 1;
@@ -614,7 +615,7 @@ sql_atoi64(const char *z, int64_t *val, int length)
 
 	char* end = NULL;
 	u64 u = strtoull(z, &end, 10);
-	if (end == z)
+	if (end < expected_end)
 		return ATOI_OVERFLOW;
 	if (errno == ERANGE)
 		return ATOI_OVERFLOW;
-- 
2.17.1

[tarantool-patches] Re: [PATCH 11/13] sql: fixes an error in string to int64 conversion

[n.pettik]

See comments to the first patch.
Should be skipped or squashed with first patch.

> Returns an error if length of the recognized 
> string is less than length of the token.
> ---
> src/box/sql/util.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index 980932f3b..17268aaaa 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -599,6 +599,7 @@ sqlAtoF(const char *z, double *pResult, int length)
> enum atoi_result
> sql_atoi64(const char *z, int64_t *val, int length)
> {
> +	const char* expected_end = z + length;
> 	int neg = 0;		/* assume positive */
> 	const char *zEnd = z + length;
> 	int incr = 1;
> @@ -614,7 +615,7 @@ sql_atoi64(const char *z, int64_t *val, int length)
> 
> 	char* end = NULL;
> 	u64 u = strtoull(z, &end, 10);
> -	if (end == z)
> +	if (end < expected_end)
> 		return ATOI_OVERFLOW;
> 	if (errno == ERANGE)
> 		return ATOI_OVERFLOW;
> -- 
> 2.17.1
> 
> 

[PATCH 12/13] sql: fixes an error in uint64 to double casting

[Stanislav Zudin]

---
 src/box/sql/util.c    | 2 +-
 src/box/sql/vdbeInt.h | 2 ++
 src/box/sql/vdbemem.c | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 17268aaaa..d585dc0d5 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -1318,7 +1318,7 @@ sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
 			if (sum == INT64_MIN_MOD) {
 				*pA = INT64_MIN;
 			} else {
-				assert(sum < INT64_MAX);
+				assert(sum <= INT64_MAX);
 				*pA = -(i64)sum;
 			}
 			return ATHR_SIGNED;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 42f22df52..2076a9a14 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -251,6 +251,8 @@ struct Mem {
 #define MEM_Unsigned  0x20000	/* Value is unsigned integer.
 				 * Combine this flag with MEM_Int
 				 * if necessary */
+#define MEM_UInt (MEM_Int | MEM_Unsigned)
+
 #ifdef SQL_OMIT_INCRBLOB
 #undef MEM_Zero
 #define MEM_Zero 0x0000
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2805d7a01..9e6d52b47 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -489,7 +489,7 @@ sqlVdbeRealValue(Mem * pMem, double *v)
 	if (pMem->flags & MEM_Real) {
 		*v = pMem->u.r;
 		return 0;
-	} else if (pMem->flags & (MEM_Int | MEM_Unsigned)) {
+	} else if ((pMem->flags & MEM_UInt) == MEM_UInt) {
 		*v = (double)(u64)pMem->u.i;
 		return 0;
 	} else if (pMem->flags & MEM_Int) {
-- 
2.17.1

[tarantool-patches] Re: [PATCH 12/13] sql: fixes an error in uint64 to double casting

[n.pettik]

Merge with one of previous patches.

> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> ---
> src/box/sql/util.c    | 2 +-
> src/box/sql/vdbeInt.h | 2 ++
> src/box/sql/vdbemem.c | 2 +-
> 3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index 17268aaaa..d585dc0d5 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -1318,7 +1318,7 @@ sqlAddInt64(i64 * pA, bool is_signedA, i64 iB, bool is_signedB)
> 			if (sum == INT64_MIN_MOD) {
> 				*pA = INT64_MIN;
> 			} else {
> -				assert(sum < INT64_MAX);
> +				assert(sum <= INT64_MAX);
> 				*pA = -(i64)sum;
> 			}
> 			return ATHR_SIGNED;
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index 42f22df52..2076a9a14 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -251,6 +251,8 @@ struct Mem {
> #define MEM_Unsigned  0x20000	/* Value is unsigned integer.
> 				 * Combine this flag with MEM_Int
> 				 * if necessary */
> +#define MEM_UInt (MEM_Int | MEM_Unsigned)
> +

IMHO it is a terrible mess. Don’t use such macros.
If MEM_Unsigned can serve only as an attribute of
MEM_Int, then you could declare its value already
containing necessary bit.

[PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type

[Stanislav Zudin]

Fixes tests

Closes #3810
---
 test/sql-tap/func.test.lua               |  6 +++---
 test/sql-tap/hexlit.test.lua             |  6 ++++--
 test/sql/gh-2347-max-int-literals.result |  2 +-
 test/sql/integer-overflow.result         | 10 +++++-----
 test/sql/iproto.result                   | 11 +++++++----
 test/sql/iproto.test.lua                 |  5 ++---
 6 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 889fc5867..8e75f9c89 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1591,14 +1591,14 @@ test:do_execsql_test(
         -- </func-18.11>
     })
 
-test:do_catchsql_test(
+test:do_execsql_test(
     "func-18.12",
     [[
         INSERT INTO t6 VALUES(3, 1<<62);
         SELECT sum(x) - ((1<<62)*2.0+1) from t6;
     ]], {
         -- <func-18.12>
-        1, "integer overflow"
+        0
         -- </func-18.12>
     })
 
@@ -1653,7 +1653,7 @@ test:do_catchsql_test(
             SELECT 10 AS x);
     ]], {
         -- <func-18.15>
-        1, "integer overflow"
+        0, {9223372036854775817ULL}
         -- </func-18.15>
     })
 
diff --git a/test/sql-tap/hexlit.test.lua b/test/sql-tap/hexlit.test.lua
index 158eda73b..1597d4b8a 100755
--- a/test/sql-tap/hexlit.test.lua
+++ b/test/sql-tap/hexlit.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(128)
+test:plan(130)
 
 --!./tcltestrunner.lua
 -- 2014-07-23
@@ -91,7 +91,9 @@ hexlit1(160, "0X1000000000000000", 1152921504606846976LL)
 hexlit1(161, "0x2000000000000000", 2305843009213693952LL)
 hexlit1(162, "0X4000000000000000", 4611686018427387904LL)
 hexlit1(163, "0x8000000000000000", -9223372036854775808LL)
-hexlit1(164, "0XFFFFFFFFFFFFFFFF", -1)
+hexlit1(164, "0x8000000000000000", 9223372036854775808ULL)
+hexlit1(165, "0x8000000000000001", 9223372036854775809ULL)
+hexlit1(166, "0XFFFFFFFFFFFFFFFF", 18446744073709551615ULL)
 for n = 1, 0x10 -1, 1 do
     hexlit1("200."..n..".1", "0X"..string.format("%03X",n), n)
     hexlit1("200."..n..".2", "0x"..string.format("%03X",n), n)
diff --git a/test/sql/gh-2347-max-int-literals.result b/test/sql/gh-2347-max-int-literals.result
index c289a80fe..e6f78d244 100644
--- a/test/sql/gh-2347-max-int-literals.result
+++ b/test/sql/gh-2347-max-int-literals.result
@@ -20,7 +20,7 @@ box.sql.execute("select (-9223372036854775808)")
 ...
 box.sql.execute("select (9223372036854775808)")
 ---
-- error: 'oversized integer: 9223372036854775808'
+- - [9223372036854775808]
 ...
 box.sql.execute("select (-9223372036854775809)")
 ---
diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result
index 4754c046c..aa9fabf1b 100644
--- a/test/sql/integer-overflow.result
+++ b/test/sql/integer-overflow.result
@@ -16,7 +16,7 @@ box.sql.execute('SELECT (2147483647 * 2147483647 * 2147483647);')
 ...
 box.sql.execute('SELECT (-9223372036854775808 / -1);')
 ---
-- error: 'Failed to execute SQL statement: integer is overflowed'
+- - [9223372036854775808]
 ...
 box.sql.execute('SELECT (-9223372036854775808 - 1);')
 ---
@@ -24,13 +24,13 @@ box.sql.execute('SELECT (-9223372036854775808 - 1);')
 ...
 box.sql.execute('SELECT (9223372036854775807 + 1);')
 ---
-- error: 'Failed to execute SQL statement: integer is overflowed'
+- - [9223372036854775808]
 ...
 -- Literals are checked right after parsing.
 --
 box.sql.execute('SELECT 9223372036854775808;')
 ---
-- error: 'oversized integer: 9223372036854775808'
+- - [9223372036854775808]
 ...
 box.sql.execute('SELECT -9223372036854775809;')
 ---
@@ -38,7 +38,7 @@ box.sql.execute('SELECT -9223372036854775809;')
 ...
 box.sql.execute('SELECT 9223372036854775808 - 1;')
 ---
-- error: 'oversized integer: 9223372036854775808'
+- - [9223372036854775807]
 ...
 -- Test that CAST may also leads to overflow.
 --
@@ -69,7 +69,7 @@ box.space.T:insert({9223372036854775809})
 ...
 box.sql.execute('SELECT * FROM t;')
 ---
-- error: 'Failed to execute SQL statement: integer is overflowed'
+- - [9223372036854775808]
 ...
 box.space.T:drop()
 ---
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index da7b40f22..deb0c5309 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -382,11 +382,14 @@ cn:execute(sql, parameters)
 --
 -- Errors during parameters binding.
 --
--- Try value > INT64_MAX. sql can't bind it, since it has no
--- suitable method in its bind API.
-cn:execute('select ? as big_uint', {0xefffffffffffffff})
+-- Try value > INT64_MAX.
+cn:execute('select ? as big_uint', {0xefffffffffffffffULL})
 ---
-- error: Bind value for parameter 1 is out of range for type INTEGER
+- metadata:
+  - name: BIG_UINT
+    type: INTEGER
+  rows:
+  - [17293822569102704639]
 ...
 -- Bind incorrect parameters.
 cn:execute('select ?', { {1, 2, 3} })
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index fbdc5a2ae..c2c3b12e5 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -112,9 +112,8 @@ cn:execute(sql, parameters)
 --
 -- Errors during parameters binding.
 --
--- Try value > INT64_MAX. sql can't bind it, since it has no
--- suitable method in its bind API.
-cn:execute('select ? as big_uint', {0xefffffffffffffff})
+-- Try value > INT64_MAX.
+cn:execute('select ? as big_uint', {0xefffffffffffffffULL})
 -- Bind incorrect parameters.
 cn:execute('select ?', { {1, 2, 3} })
 parameters = {}
-- 
2.17.1

[tarantool-patches] Re: [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type

[n.pettik]

Merge this patch with the main one in your patch-set.
Also, please add sufficient number of tests verifying that
INT in a range of [2^63, 2^64 - 1] is working without as designed

> Closes #3810
> ---
> test/sql-tap/func.test.lua               |  6 +++---
> test/sql-tap/hexlit.test.lua             |  6 ++++--
> test/sql/gh-2347-max-int-literals.result |  2 +-
> test/sql/integer-overflow.result         | 10 +++++-----
> test/sql/iproto.result                   | 11 +++++++----
> test/sql/iproto.test.lua                 |  5 ++---
> 6 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
> index 889fc5867..8e75f9c89 100755
> --- a/test/sql-tap/func.test.lua
> +++ b/test/sql-tap/func.test.lua
> @@ -1591,14 +1591,14 @@ test:do_execsql_test(
>         -- </func-18.11>
>     })
> 
> -test:do_catchsql_test(
> +test:do_execsql_test(
>     "func-18.12",
>     [[
>         INSERT INTO t6 VALUES(3, 1<<62);
>         SELECT sum(x) - ((1<<62)*2.0+1) from t6;
>     ]], {
>         -- <func-18.12>
> -        1, "integer overflow"
> +        0


tarantool>         SELECT sum(x)  from t6;
---
- - [9223372036854775809]
...

tarantool>         SELECT ((1<<62)*2.0+1)  from t6;
---
- - [9223372036854775808]
  - [9223372036854775808]
  - [9223372036854775808]
…

So, how it could be that SELECT sum(x) - ((1<<62)*2.0+1) is 0?


What is more, I see this:

tarantool>         SELECT typeof(sum(x))  from t6;
---
- - ['null']
…

Which is obviously wrong.


> diff --git a/test/sql-tap/hexlit.test.lua b/test/sql-tap/hexlit.test.lua
> index 158eda73b..1597d4b8a 100755
> --- a/test/sql-tap/hexlit.test.lua
> +++ b/test/sql-tap/hexlit.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(128)
> +test:plan(130)
> 
> --!./tcltestrunner.lua
> -- 2014-07-23
> @@ -91,7 +91,9 @@ hexlit1(160, "0X1000000000000000", 1152921504606846976LL)
> hexlit1(161, "0x2000000000000000", 2305843009213693952LL)
> hexlit1(162, "0X4000000000000000", 4611686018427387904LL)
> hexlit1(163, "0x8000000000000000", -9223372036854775808LL)
> -hexlit1(164, "0XFFFFFFFFFFFFFFFF", -1)
> +hexlit1(164, "0x8000000000000000", 9223372036854775808ULL)
> +hexlit1(165, "0x8000000000000001", 9223372036854775809ULL)
> +hexlit1(166, "0XFFFFFFFFFFFFFFFF", 18446744073709551615ULL)
> for n = 1, 0x10 -1, 1 do
>     hexlit1("200."..n..".1", "0X"..string.format("%03X",n), n)
>     hexlit1("200."..n..".2", "0x"..string.format("%03X",n), n)
> diff --git a/test/sql/gh-2347-max-int-literals.result b/test/sql/gh-2347-max-int-literals.result
> index c289a80fe..e6f78d244 100644
> --- a/test/sql/gh-2347-max-int-literals.result
> +++ b/test/sql/gh-2347-max-int-literals.result
> @@ -20,7 +20,7 @@ box.sql.execute("select (-9223372036854775808)")
> ...
> box.sql.execute("select (9223372036854775808)")
> ---
> -- error: 'oversized integer: 9223372036854775808'
> +- - [9223372036854775808]

Please, make these test check that overflow error is handled,
not simply fixing result file.

[tarantool-patches] Re: [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type

[Stanislav Zudin]

On 25.03.2019 18:25, n.pettik wrote:
> Merge this patch with the main one in your patch-set.
> Also, please add sufficient number of tests verifying that
> INT in a range of [2^63, 2^64 - 1] is working without as designed
> 
>> Closes #3810
>> ---
>> test/sql-tap/func.test.lua               |  6 +++---
>> test/sql-tap/hexlit.test.lua             |  6 ++++--
>> test/sql/gh-2347-max-int-literals.result |  2 +-
>> test/sql/integer-overflow.result         | 10 +++++-----
>> test/sql/iproto.result                   | 11 +++++++----
>> test/sql/iproto.test.lua                 |  5 ++---
>> 6 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
>> index 889fc5867..8e75f9c89 100755
>> --- a/test/sql-tap/func.test.lua
>> +++ b/test/sql-tap/func.test.lua
>> @@ -1591,14 +1591,14 @@ test:do_execsql_test(
>>          -- </func-18.11>
>>      })
>>
>> -test:do_catchsql_test(
>> +test:do_execsql_test(
>>      "func-18.12",
>>      [[
>>          INSERT INTO t6 VALUES(3, 1<<62);
>>          SELECT sum(x) - ((1<<62)*2.0+1) from t6;
>>      ]], {
>>          -- <func-18.12>
>> -        1, "integer overflow"
>> +        0
> 
> 
> tarantool>         SELECT sum(x)  from t6;
> ---
> - - [9223372036854775809]
> ...
> 
> tarantool>         SELECT ((1<<62)*2.0+1)  from t6;
> ---
> - - [9223372036854775808]
>    - [9223372036854775808]
>    - [9223372036854775808]
> …
> 
> So, how it could be that SELECT sum(x) - ((1<<62)*2.0+1) is 0?
> 

The "func-18.12" uses data inserted in "func-18.10".
The "(1<<62)" was inserted twice.

tarantool> box.sql.execute("SELECT * from t6;")
---
- - [1, 1]
   - [2, 4611686018427387904]
   - [3, 4611686018427387904]
...

tarantool> box.sql.execute("SELECT sum(x) from t6;")
---
- - [9223372036854775809]
...

tarantool> box.sql.execute("SELECT sum(x) - ((1<<62)*2.0+1) from t6;")
---
- - [0]
...

This test is definitely correct.

> 
> What is more, I see this:
> 
> tarantool>         SELECT typeof(sum(x))  from t6;
> ---
> - - ['null']
> …
> 
> Which is obviously wrong.

This was a bug. Fixed.

> 
> 
>> diff --git a/test/sql-tap/hexlit.test.lua b/test/sql-tap/hexlit.test.lua
>> index 158eda73b..1597d4b8a 100755
>> --- a/test/sql-tap/hexlit.test.lua
>> +++ b/test/sql-tap/hexlit.test.lua
>> @@ -1,6 +1,6 @@
>> #!/usr/bin/env tarantool
>> test = require("sqltester")
>> -test:plan(128)
>> +test:plan(130)
>>
>> --!./tcltestrunner.lua
>> -- 2014-07-23
>> @@ -91,7 +91,9 @@ hexlit1(160, "0X1000000000000000", 1152921504606846976LL)
>> hexlit1(161, "0x2000000000000000", 2305843009213693952LL)
>> hexlit1(162, "0X4000000000000000", 4611686018427387904LL)
>> hexlit1(163, "0x8000000000000000", -9223372036854775808LL)
>> -hexlit1(164, "0XFFFFFFFFFFFFFFFF", -1)
>> +hexlit1(164, "0x8000000000000000", 9223372036854775808ULL)
>> +hexlit1(165, "0x8000000000000001", 9223372036854775809ULL)
>> +hexlit1(166, "0XFFFFFFFFFFFFFFFF", 18446744073709551615ULL)
>> for n = 1, 0x10 -1, 1 do
>>      hexlit1("200."..n..".1", "0X"..string.format("%03X",n), n)
>>      hexlit1("200."..n..".2", "0x"..string.format("%03X",n), n)
>> diff --git a/test/sql/gh-2347-max-int-literals.result b/test/sql/gh-2347-max-int-literals.result
>> index c289a80fe..e6f78d244 100644
>> --- a/test/sql/gh-2347-max-int-literals.result
>> +++ b/test/sql/gh-2347-max-int-literals.result
>> @@ -20,7 +20,7 @@ box.sql.execute("select (-9223372036854775808)")
>> ...
>> box.sql.execute("select (9223372036854775808)")
>> ---
>> -- error: 'oversized integer: 9223372036854775808'
>> +- - [9223372036854775808]
> 
> Please, make these test check that overflow error is handled,
> not simply fixing result file.
> 
> 
Done.
> 

[tarantool-patches] Re: [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type

[n.pettik]

> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> The patch enables support of big integers in the range [2^63, 2^64-1].
> Conversion functions use return value to inform about value
> they return - signed integer in the range [-2^63,2^63-1] or 
> unsigned integer in the range [2^63, 2^64 -1].
> The changes affect sql processing, VDBE, arithmetic functions and
> aggregate functions.

I see a lot of commits below, so it would be nice to see
some details concerning your plan of implementation.

> Issue: https://github.com/tarantool/tarantool/issues/3810
> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3810-sql-uint64-support

I didn’t dive into your patches yet, but I’ve come up with
simple and strange cases:

tarantool> CREATE TABLE t (id INT PRIMARY KEY, a INT);
tarantool> box.space.T:insert{1, 18446744073709551615ULL}
---
- [1, 18446744073709551615]
…
tarantool> SELECT a*1 FROM t;
---
- error: 'Failed to execute SQL statement: integer is overflowed'
...
tarantool> select a from t;
---
- - [18446744073709551615]
…

Or:

tarantool> select -9223372036854775808*(-1)
---
- error: 'Failed to execute SQL statement: integer is overflowed'
...

Second example:

tarantool> format = {}
tarantool> format[1] = {'a', 'integer'}
tarantool> format[2] = {'b', 'unsigned’}

tarantool> box.schema.space.create('T1', {format = format})
tarantool> box.space.T1:create_index('pk', {parts = {{'a'}}})

tarantool> box.sql.execute("insert into t1 values(18446744073709551615, 18446744073709551615)")
---
...

tarantool> box.sql.execute("select * from t1")
---
- - [-1, 18446744073709551615]
…

I expected that mentioned insertion would be equal to this one:

box.space.T1:insert({18446744073709551615ULL, 18446744073709551615ULL})

But instead, it is the same as:

box.space.T1:insert({18446744073709551615LL, 18446744073709551615ULL})

[tarantool-patches] Re: [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type

[Stanislav Zudin]

On 25.03.2019 18:10, n.pettik wrote:
> 
> 
>> On 15 Mar 2019, at 18:45, Stanislav Zudin <szudin@tarantool.org> wrote:
>>
>> The patch enables support of big integers in the range [2^63, 2^64-1].
>> Conversion functions use return value to inform about value
>> they return - signed integer in the range [-2^63,2^63-1] or
>> unsigned integer in the range [2^63, 2^64 -1].
>> The changes affect sql processing, VDBE, arithmetic functions and
>> aggregate functions.
> 
> I see a lot of commits below, so it would be nice to see
> some details concerning your plan of implementation.
> 
>> Issue: https://github.com/tarantool/tarantool/issues/3810
>> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3810-sql-uint64-support
> 
> I didn’t dive into your patches yet, but I’ve come up with
> simple and strange cases:
> 
> tarantool> CREATE TABLE t (id INT PRIMARY KEY, a INT);
> tarantool> box.space.T:insert{1, 18446744073709551615ULL}
> ---
> - [1, 18446744073709551615]
> …
> tarantool> SELECT a*1 FROM t;
> ---
> - error: 'Failed to execute SQL statement: integer is overflowed'
> ...
> tarantool> select a from t;
> ---
> - - [18446744073709551615]
> …
> 
> Or:
> 
> tarantool> select -9223372036854775808*(-1)
> ---
> - error: 'Failed to execute SQL statement: integer is overflowed'
> ...

Fixed.

> 
> Second example:
> 
> tarantool> format = {}
> tarantool> format[1] = {'a', 'integer'}
> tarantool> format[2] = {'b', 'unsigned’}
> 
> tarantool> box.schema.space.create('T1', {format = format})
> tarantool> box.space.T1:create_index('pk', {parts = {{'a'}}})
> 
> tarantool> box.sql.execute("insert into t1 values(18446744073709551615, 18446744073709551615)")
> ---
> ...
> 
> tarantool> box.sql.execute("select * from t1")
> ---
> - - [-1, 18446744073709551615]
> …

Fixed.
The correct output is following:

tarantool> box.sql.execute("select * from t1")
---
- - [18446744073709551615, 18446744073709551615]
...


> 
> I expected that mentioned insertion would be equal to this one:
> 
> box.space.T1:insert({18446744073709551615ULL, 18446744073709551615ULL})
> 
> But instead, it is the same as:
> 
> box.space.T1:insert({18446744073709551615LL, 18446744073709551615ULL})
> 
> 
> 
>