From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 1/6] sql: refactor sql_atoi64() Date: Mon, 1 Jul 2019 17:20:56 +0300 [thread overview] Message-ID: <9898E449-3735-4744-83A9-47B1DC64A72C@tarantool.org> (raw) In-Reply-To: <0ae708be-9670-e726-b7ea-9faec8c007ea@tarantool.org> >> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >> index ba7eea59d..158631416 100644 >> --- a/src/box/sql/expr.c >> +++ b/src/box/sql/expr.c> @@ -3325,29 +3326,53 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg, >> if (is_neg) >> i = -i; >> sqlVdbeAddOp2(v, OP_Integer, i, mem); >> + return; >> + } >> + int64_t value; >> + const char *z = expr->u.zToken; >> + assert(z != NULL); >> + const char *sign = is_neg ? "-" : ""; >> + if (z[0] == '0' && (z[1] == 'x' || z[1] == 'X')) { >> + uint64_t u = 0; >> + int i, k; >> + for (i = 2; z[i] == '0'; i++); >> + for (k = i; sqlIsxdigit(z[k]); k++) >> + u = u * 16 + sqlHexToInt(z[k]); > > 1. Why not to use strtoll and strtol? They accept 'base' argument. > Set it to 16, and they will do the same, but probably faster. NP: diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 158631416..182950e9f 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3333,13 +3333,16 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg, assert(z != NULL); const char *sign = is_neg ? "-" : ""; if (z[0] == '0' && (z[1] == 'x' || z[1] == 'X')) { - uint64_t u = 0; - int i, k; - for (i = 2; z[i] == '0'; i++); - for (k = i; sqlIsxdigit(z[k]); k++) - u = u * 16 + sqlHexToInt(z[k]); - memcpy(&value, &u, 8); - if (z[k] != 0 || k - i > 16) { + errno = 0; + char *end = NULL; + if (is_neg) { + value = strtoll(z, &end, 16); + } else { + value = strtoull(z, &end, 16); + if (value > INT64_MAX) + goto int_overflow; + } + if (errno != 0) { diag_set(ClientError, ER_HEX_LITERAL_MAX, sign, z, strlen(z) - 2, 16); parse->is_aborted = true; > >> + memcpy(&value, &u, 8); >> + if (z[k] != 0 || k - i > 16) { > > 2. You treat tail spaces as an error. Not sure if it is right. This code has been removed (see fix above). > >> + diag_set(ClientError, ER_HEX_LITERAL_MAX, sign, z, >> + strlen(z) - 2, 16); >> + parse->is_aborted = true; >> + return; >> + } >> } else { >> - 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)) { >> - const char *sign = is_neg ? "-" : ""; >> - if (sql_strnicmp(z, "0x", 2) == 0) { >> - diag_set(ClientError, ER_HEX_LITERAL_MAX, sign, >> - z, strlen(z) - 2, 16); >> - } else { >> - diag_set(ClientError, ER_INT_LITERAL_MAX, sign, >> - z, INT64_MIN, INT64_MAX); >> - } >> + size_t len = strlen(z); >> + if (is_neg) { >> + /* >> + * UINT64_MAX is constant, so each integer >> + * literal certainly contains less digits. >> + * It makes no sense to continue processing >> + * string if it contains more characters. >> + */ >> + if (len > UINT64_MAX_DIGIT_COUNT) >> + goto int_overflow; >> + char *neg_z = tt_static_buf(); >> + neg_z[0] = '-'; >> + memcpy(neg_z + 1, z, len++); >> + neg_z[len]= '\0'; >> + z = neg_z; >> + } > > 3. This looks like a crutch. Why do you need strlen() and number of > digits here? Firstly, sql_atoi64 already returns an error in case of > too long integer. Secondly, if you wanted to add '-', it could be done > much faster - make 'value = -value;' in case of 'neg' with a preliminary > check that the value is not too big. Will it work? It will, but we have to allow parsing in sql_atoi() positive values up to INT64_MAX + 1. So, several tests fail with this change (sql-tap/func.test.lua and sql/integer-overflow.test.lua). It is not critical, since in the next patch we are going to allow using integers up to UINT64_MAX. Hence, I don’t think this deserves any workaround, so I simply fixed tests. Diff: --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -242,7 +242,7 @@ struct errcode_record { /*187 */_(ER_SQL_ANALYZE_ARGUMENT, "ANALYZE statement argument %s is not a base table") \ /*188 */_(ER_SQL_COLUMN_COUNT_MAX, "Failed to create space '%s': space column count %d exceeds the limit (%d)") \ /*189 */_(ER_HEX_LITERAL_MAX, "Hex literal %s%s length %d exceeds the supported limit (%d)") \ - /*190 */_(ER_INT_LITERAL_MAX, "Integer literal %s exceeds the supported range %lld - %lld") \ + /*190 */_(ER_INT_LITERAL_MAX, "Integer literal %s%s exceeds the supported range %lld - %lld") \ /*191 */_(ER_SQL_PARSER_LIMIT, "%s %d exceeds the limit (%d)") \ /*192 */_(ER_INDEX_DEF_UNSUPPORTED, "%s are prohibited in an index definition") \ /*193 */_(ER_CK_DEF_UNSUPPORTED, "%s are prohibited in a CHECK constraint definition") \ diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 158631416..4bb5ac180 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3347,31 +3350,19 @@ expr_code_int(struct Parse *parse, struct Expr *expr, bool is_neg, } } else { size_t len = strlen(z); - if (is_neg) { - /* - * UINT64_MAX is constant, so each integer - * literal certainly contains less digits. - * It makes no sense to continue processing - * string if it contains more characters. - */ - if (len > UINT64_MAX_DIGIT_COUNT) - goto int_overflow; - char *neg_z = tt_static_buf(); - neg_z[0] = '-'; - memcpy(neg_z + 1, z, len++); - neg_z[len]= '\0'; - z = neg_z; - } bool unused; - int rc = sql_atoi64(z, &value, &unused, len); - if (rc != 0) { + if (sql_atoi64(z, &value, &unused, len) != 0 || + (is_neg && (uint64_t) value > (uint64_t) INT64_MAX + 1) || + (!is_neg && (uint64_t) value > INT64_MAX)) { int_overflow: - diag_set(ClientError, ER_INT_LITERAL_MAX, z, INT64_MIN, - INT64_MAX); + diag_set(ClientError, ER_INT_LITERAL_MAX, sign, z, + INT64_MIN, INT64_MAX); parse->is_aborted = true; return; } } + if (is_neg) + value = -value; sqlVdbeAddOp4Dup8(v, OP_Int64, 0, mem, 0, (u8 *)&value, P4_INT64); } diff --git a/src/box/sql/util.c b/src/box/sql/util.c index 2e7c298e1..a2340f001 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -611,7 +611,7 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length) *val = strtoll(z, &end, 10); } else { uint64_t u_val = strtoull(z, &end, 10); - if (u_val > INT64_MAX) + if (u_val > (uint64_t) INT64_MAX + 1) return -1; *val = u_val; } diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua index 09b1cf967..6cb14f3a3 100755 --- a/test/sql-tap/func.test.lua +++ b/test/sql-tap/func.test.lua @@ -912,25 +912,25 @@ test:do_execsql_test( -- </func-8.6> }) -test:do_execsql_test( +test:do_catchsql_test( "func-8.7", [[ SELECT typeof(sum(x)) FROM (SELECT '9223372036' || '854775808' AS x UNION ALL SELECT -9223372036854775807) ]], { -- <func-8.7> - "number" + 1, "Failed to execute SQL statement: integer overflow" -- </func-8.7> }) -test:do_execsql_test( +test:do_catchsql_test( "func-8.8", [[ SELECT sum(x)>0.0 FROM (SELECT '9223372036' || '854775808' AS x UNION ALL SELECT -9223372036850000000) ]], { -- <func-8.8> - 1 + 1, "Failed to execute SQL statement: integer overflow" -- </func-8.8> }) diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result index d6ca66ec9..a4944d1a0 100644 --- a/test/sql/integer-overflow.result +++ b/test/sql/integer-overflow.result @@ -48,7 +48,11 @@ box.execute('SELECT 9223372036854775808 - 1;') -- box.execute('SELECT CAST(\'9223372036854775808\' AS INTEGER);') --- -- error: 'Type mismatch: can not convert 9223372036854775808 to integer' +- metadata: + - name: CAST('9223372036854775808' AS INTEGER) + type: integer + rows: + - [-9223372036854775808] ... -- Due to inexact represantation of large integers in terms of -- floating point numbers, numerics with value < INT64_MAX >> }> diff --git a/src/box/sql/util.c b/src/box/sql/util.c >> index d5c93f8aa..2e7c298e1 100644 >> --- a/src/box/sql/util.c >> +++ b/src/box/sql/util.c >> @@ -593,120 +593,42 @@ sqlAtoF(const char *z, double *pResult, int length) >> int >> -sql_atoi64(const char *z, int64_t *val, int length) >> +sql_atoi64(const char *z, int64_t *val, bool *is_neg, 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; >> - 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') { >> - 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; >> + *is_neg = false; >> + const char *str_end = z + length; >> + for (; z < str_end && sqlIsspace(*z); z++) >> + /* invalid format */ > > 4. Usually we start sentences from capital letters and finish > with dots. Refactored a bit whole function (almost applied your diff): diff --git a/src/box/sql/util.c b/src/box/sql/util.c index 2e7c298e1..826e4a673 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -38,6 +38,7 @@ */ #include "sqlInt.h" #include <stdarg.h> +#include <ctype.h> #if HAVE_ISNAN || SQL_HAVE_ISNAN #include <math.h> #endif @@ -598,26 +599,24 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length) { *is_neg = false; const char *str_end = z + length; - for (; z < str_end && sqlIsspace(*z); z++) - /* invalid format */ + for (; z < str_end && isspace(*z); z++); if (z >= str_end) return -1; if (*z == '-') *is_neg = true; - char* end = NULL; + char *end = NULL; errno = 0; - if (*is_neg) { + if (*z == '-') { + *is_neg = true; *val = strtoll(z, &end, 10); } else { + *is_neg = false; uint64_t u_val = strtoull(z, &end, 10); - if (u_val > INT64_MAX) + if (u_val > (uint64_t) INT64_MAX + 1) return -1; *val = u_val; } - /* No digits were found, e.g. an empty string. */ - if (end == z) - return -1; >> + if (z >= str_end) >> + return -1; >> + if (*z == '-') >> + *is_neg = true; >> + >> + char* end = NULL; >> + errno = 0; >> + if (*is_neg) { > > 5. I don't like this double check. 4 lines above you already > know if the value is negative, but you double check it here. Ok, see diff above. >> + *val = strtoll(z, &end, 10); >> } else { >> - *val = (i64) u; >> + uint64_t u_val = strtoull(z, &end, 10); >> + if (u_val > INT64_MAX) >> + return -1; >> + *val = u_val; >> } >> - 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; >> - } >> - } >> -} >> + /* No digits were found, e.g. an empty string. */ >> + if (end == z) >> + return -1; > > 6. You have already checked emptiness above with > this condition: 'if (z >= str_end)’. Indeed, see diff above. >> + /* >> + * String has been parsed, but there's >> + * additional character(s) remained. >> + */ >> + if (*end != '\0') >> + return -1; > > 7. What if other symbols are spaces? So, you are speaking about situation like: SELECT CAST(“123 “ AS INTEGER); According to ANSI cast is allowed in this case: … 8) If TD is exact numeric, then … b) If SD is character string, then SV is replaced by SV with any leading or trailing s removed. Hence, you are right, we should skip trailing space as well. Applied your diff: diff --git a/src/box/sql/util.c b/src/box/sql/util.c index 826e4a673..e73fe3f87 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -618,21 +618,18 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length) *val = u_val; } - /* - * String has been parsed, but there's - * additional character(s) remained. - */ - if (*end != '\0') - return -1; /* Overflow and underflow errors. */ if (errno != 0) return -1; + for (; *end != 0; ++end) { + if (! isspace(*end)) + return -1; + } return 0; } >> + /* Overflow and underflow errors. */ >> + if (errno != 0) >> + return -1; > > 8. You didn't remove 'sql_atoi64() == SQL_OK' in some > places. Thx, applied this part of your diff. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 6ecdb26fc..11a92372b 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -386,7 +386,7 @@ static u16 SQL_NOINLINE computeNumericType(Mem *pMem) > if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n)==0) > return 0; > bool is_neg; > - if (sql_atoi64(pMem->z, (int64_t *)&pMem->u.i, &is_neg, pMem->n)==SQL_OK) > + if (sql_atoi64(pMem->z, &pMem->u.i, &is_neg, pMem->n) == 0) This cast is required.. Otherwise on linux compilation error is raised: /tarantool/src/box/sql/vdbe.c:387:26: error: passing argument 2 of ‘sql_atoi64’ from incompatible pointer type [-Werror=incompatible-pointer-types] if (sql_atoi64(pMem->z, &pMem->u.i, &is_neg, pMem->n) == 0) ^ In file included from /tarantool/src/box/sql/vdbe.c:47:0: /tarantool/src/box/sql/sqlInt.h:4435:1: note: expected ‘int64_t * {aka long int *}’ but argument is of type ‘i64 * {aka long long int *}’ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length);
next prev parent reply other threads:[~2019-07-01 14:21 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-07 15:37 [tarantool-patches] [PATCH 0/6] Introduce UNSIGNED type in SQL Nikita Pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 1/6] sql: refactor sql_atoi64() Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:20 ` n.pettik [this message] 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:32 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 2/6] sql: separate VDBE memory holding positive and negative ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:33 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 3/6] sql: refactor arithmetic operations to support unsigned ints Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 12:24 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 4/6] sql: make built-in functions operate on unsigned values Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-06-07 15:37 ` [tarantool-patches] [PATCH 5/6] sql: introduce extended range for INTEGER type Nikita Pettik 2019-06-11 21:11 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-01 14:21 ` n.pettik 2019-07-01 21:53 ` Vladislav Shpilevoy 2019-07-24 15:59 ` Konstantin Osipov 2019-07-24 16:54 ` n.pettik 2019-07-24 17:09 ` Konstantin Osipov 2019-06-07 15:37 ` [tarantool-patches] [PATCH 6/6] sql: allow to specify UNSIGNED column type Nikita Pettik 2019-07-01 21:53 ` [tarantool-patches] " Vladislav Shpilevoy 2019-07-05 16:36 ` n.pettik 2019-07-10 22:49 ` Vladislav Shpilevoy 2019-07-11 21:25 ` Vladislav Shpilevoy 2019-07-17 0:53 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-07-18 20:56 ` n.pettik 2019-07-18 21:08 ` Vladislav Shpilevoy 2019-07-18 21:13 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-22 19:17 ` Vladislav Shpilevoy 2019-07-22 10:20 ` n.pettik 2019-07-17 0:54 ` n.pettik 2019-07-18 20:18 ` Vladislav Shpilevoy 2019-08-06 19:36 ` n.pettik 2019-07-24 13:01 ` [tarantool-patches] Re: [PATCH 0/6] Introduce UNSIGNED type in SQL Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9898E449-3735-4744-83A9-47B1DC64A72C@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 1/6] sql: refactor sql_atoi64()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox