* [PATCH 01/13] sql: Convert big integers from string
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:10 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 02/13] sql: make VDBE recognize big integers Stanislav Zudin
` (12 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string
2019-03-15 15:45 ` [PATCH 01/13] sql: Convert big integers from string Stanislav Zudin
@ 2019-03-25 15:10 ` n.pettik
2019-04-01 20:39 ` Stanislav Zudin
2019-04-02 7:27 ` Konstantin Osipov
0 siblings, 2 replies; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:10 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string
2019-03-25 15:10 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:39 ` Stanislav Zudin
2019-04-02 7:27 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:39 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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.
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 01/13] sql: Convert big integers from string
2019-03-25 15:10 ` [tarantool-patches] " n.pettik
2019-04-01 20:39 ` Stanislav Zudin
@ 2019-04-02 7:27 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:27 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 02/13] sql: make VDBE recognize big integers
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
2019-03-15 15:45 ` [PATCH 01/13] sql: Convert big integers from string Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:11 ` [tarantool-patches] " n.pettik
2019-04-02 7:38 ` [tarantool-patches] " Konstantin Osipov
2019-03-15 15:45 ` [PATCH 03/13] sql: removes unused function Stanislav Zudin
` (11 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 02/13] sql: make VDBE recognize big integers
2019-03-15 15:45 ` [PATCH 02/13] sql: make VDBE recognize big integers Stanislav Zudin
@ 2019-03-25 15:11 ` n.pettik
2019-04-01 20:42 ` Stanislav Zudin
2019-04-02 7:38 ` [tarantool-patches] " Konstantin Osipov
1 sibling, 1 reply; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:11 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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)
> {
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 02/13] sql: make VDBE recognize big integers
2019-03-25 15:11 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:42 ` Stanislav Zudin
0 siblings, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:42 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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)
>> {
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [tarantool-patches] [PATCH 02/13] sql: make VDBE recognize big integers
2019-03-15 15:45 ` [PATCH 02/13] sql: make VDBE recognize big integers Stanislav Zudin
2019-03-25 15:11 ` [tarantool-patches] " n.pettik
@ 2019-04-02 7:38 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:38 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev, Stanislav Zudin
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 03/13] sql: removes unused function.
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
2019-03-15 15:45 ` [PATCH 01/13] sql: Convert big integers from string Stanislav Zudin
2019-03-15 15:45 ` [PATCH 02/13] sql: make VDBE recognize big integers Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:11 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 04/13] sql: support big integers within sql binding Stanislav Zudin
` (10 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 03/13] sql: removes unused function.
2019-03-15 15:45 ` [PATCH 03/13] sql: removes unused function Stanislav Zudin
@ 2019-03-25 15:11 ` n.pettik
2019-04-01 20:39 ` Stanislav Zudin
0 siblings, 1 reply; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:11 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 03/13] sql: removes unused function.
2019-03-25 15:11 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:39 ` Stanislav Zudin
0 siblings, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:39 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 04/13] sql: support big integers within sql binding
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (2 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 03/13] sql: removes unused function Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:12 ` [tarantool-patches] " n.pettik
2019-04-02 7:44 ` [tarantool-patches] " Konstantin Osipov
2019-03-15 15:45 ` [PATCH 05/13] sql: removes redundant function Stanislav Zudin
` (9 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 04/13] sql: support big integers within sql binding
2019-03-15 15:45 ` [PATCH 04/13] sql: support big integers within sql binding Stanislav Zudin
@ 2019-03-25 15:12 ` n.pettik
2019-04-01 20:42 ` Stanislav Zudin
2019-04-02 7:46 ` Konstantin Osipov
2019-04-02 7:44 ` [tarantool-patches] " Konstantin Osipov
1 sibling, 2 replies; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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;
> }
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 04/13] sql: support big integers within sql binding
2019-03-25 15:12 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:42 ` Stanislav Zudin
2019-04-02 7:46 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:42 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 04/13] sql: support big integers within sql binding
2019-03-25 15:12 ` [tarantool-patches] " n.pettik
2019-04-01 20:42 ` Stanislav Zudin
@ 2019-04-02 7:46 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:46 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [tarantool-patches] [PATCH 04/13] sql: support big integers within sql binding
2019-03-15 15:45 ` [PATCH 04/13] sql: support big integers within sql binding Stanislav Zudin
2019-03-25 15:12 ` [tarantool-patches] " n.pettik
@ 2019-04-02 7:44 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev, Stanislav Zudin
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 05/13] sql: removes redundant function.
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (3 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 04/13] sql: support big integers within sql binding Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:12 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 06/13] sql: aux functions to support big integers Stanislav Zudin
` (8 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 06/13] sql: aux functions to support big integers
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (4 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 05/13] sql: removes redundant function Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 07/13] sql: arithmetic functions " Stanislav Zudin
` (7 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 07/13] sql: arithmetic functions support big integers
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (5 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 06/13] sql: aux functions to support big integers Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 08/13] sql: aggregate sql functions support big int Stanislav Zudin
` (6 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers
2019-03-15 15:45 ` [PATCH 07/13] sql: arithmetic functions " Stanislav Zudin
@ 2019-03-25 15:13 ` n.pettik
2019-04-01 20:43 ` Stanislav Zudin
2019-04-02 7:52 ` Konstantin Osipov
0 siblings, 2 replies; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:13 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:43 ` Stanislav Zudin
2019-04-02 7:54 ` Konstantin Osipov
2019-04-02 7:52 ` Konstantin Osipov
1 sibling, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:43 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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.
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers
2019-04-01 20:43 ` Stanislav Zudin
@ 2019-04-02 7:54 ` Konstantin Osipov
0 siblings, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:54 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 07/13] sql: arithmetic functions support big integers
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
2019-04-01 20:43 ` Stanislav Zudin
@ 2019-04-02 7:52 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:52 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 08/13] sql: aggregate sql functions support big int
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (6 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 07/13] sql: arithmetic functions " Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
2019-04-02 7:57 ` [tarantool-patches] " Konstantin Osipov
2019-03-15 15:45 ` [PATCH 09/13] sql: fixes errors Stanislav Zudin
` (5 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 08/13] sql: aggregate sql functions support big int
2019-03-15 15:45 ` [PATCH 08/13] sql: aggregate sql functions support big int Stanislav Zudin
@ 2019-03-25 15:13 ` n.pettik
2019-04-01 20:43 ` Stanislav Zudin
2019-04-02 7:57 ` [tarantool-patches] " Konstantin Osipov
1 sibling, 1 reply; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:13 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 08/13] sql: aggregate sql functions support big int
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:43 ` Stanislav Zudin
0 siblings, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:43 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [tarantool-patches] [PATCH 08/13] sql: aggregate sql functions support big int
2019-03-15 15:45 ` [PATCH 08/13] sql: aggregate sql functions support big int Stanislav Zudin
2019-03-25 15:13 ` [tarantool-patches] " n.pettik
@ 2019-04-02 7:57 ` Konstantin Osipov
1 sibling, 0 replies; 43+ messages in thread
From: Konstantin Osipov @ 2019-04-02 7:57 UTC (permalink / raw)
To: tarantool-patches; +Cc: vdavydov.dev, Stanislav Zudin
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 09/13] sql: fixes errors
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (7 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 08/13] sql: aggregate sql functions support big int Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:14 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 10/13] sql: fixes an error in sqlSubInt64 Stanislav Zudin
` (4 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 10/13] sql: fixes an error in sqlSubInt64
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (8 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 09/13] sql: fixes errors Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:14 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 11/13] sql: fixes an error in string to int64 conversion Stanislav Zudin
` (3 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 11/13] sql: fixes an error in string to int64 conversion
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (9 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 10/13] sql: fixes an error in sqlSubInt64 Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:14 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 12/13] sql: fixes an error in uint64 to double casting Stanislav Zudin
` (2 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 11/13] sql: fixes an error in string to int64 conversion
2019-03-15 15:45 ` [PATCH 11/13] sql: fixes an error in string to int64 conversion Stanislav Zudin
@ 2019-03-25 15:14 ` n.pettik
0 siblings, 0 replies; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:14 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
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
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 12/13] sql: fixes an error in uint64 to double casting
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (10 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 11/13] sql: fixes an error in string to int64 conversion Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:15 ` [tarantool-patches] " n.pettik
2019-03-15 15:45 ` [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
2019-03-25 15:10 ` [tarantool-patches] Re: [PATCH 00/13] " n.pettik
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 12/13] sql: fixes an error in uint64 to double casting
2019-03-15 15:45 ` [PATCH 12/13] sql: fixes an error in uint64 to double casting Stanislav Zudin
@ 2019-03-25 15:15 ` n.pettik
0 siblings, 0 replies; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:15 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (11 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 12/13] sql: fixes an error in uint64 to double casting Stanislav Zudin
@ 2019-03-15 15:45 ` Stanislav Zudin
2019-03-25 15:25 ` [tarantool-patches] " n.pettik
2019-03-25 15:10 ` [tarantool-patches] Re: [PATCH 00/13] " n.pettik
13 siblings, 1 reply; 43+ messages in thread
From: Stanislav Zudin @ 2019-03-15 15:45 UTC (permalink / raw)
To: tarantool-patches, vdavydov.dev; +Cc: 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type
2019-03-15 15:45 ` [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
@ 2019-03-25 15:25 ` n.pettik
2019-04-01 20:44 ` Stanislav Zudin
0 siblings, 1 reply; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:25 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type
2019-03-25 15:25 ` [tarantool-patches] " n.pettik
@ 2019-04-01 20:44 ` Stanislav Zudin
0 siblings, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:44 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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.
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type
2019-03-15 15:45 [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
` (12 preceding siblings ...)
2019-03-15 15:45 ` [PATCH 13/13] sql: support -2^63 .. 2^64-1 integer type Stanislav Zudin
@ 2019-03-25 15:10 ` n.pettik
2019-04-01 20:38 ` Stanislav Zudin
13 siblings, 1 reply; 43+ messages in thread
From: n.pettik @ 2019-03-25 15:10 UTC (permalink / raw)
To: tarantool-patches; +Cc: szudin
> 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})
^ permalink raw reply [flat|nested] 43+ messages in thread
* [tarantool-patches] Re: [PATCH 00/13] sql: support -2^63 .. 2^64-1 integer type
2019-03-25 15:10 ` [tarantool-patches] Re: [PATCH 00/13] " n.pettik
@ 2019-04-01 20:38 ` Stanislav Zudin
0 siblings, 0 replies; 43+ messages in thread
From: Stanislav Zudin @ 2019-04-01 20:38 UTC (permalink / raw)
To: tarantool-patches, n.pettik
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})
>
>
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread