[Tarantool-patches] [PATCH v1 2/8] sql: refactor CHAR_LENGTH() function
Mergen Imeev
imeevma at tarantool.org
Wed Oct 20 19:58:44 MSK 2021
Thank you for the review! My answer, diff and new patch below.
On Fri, Oct 08, 2021 at 11:56:59PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> On 01.10.2021 18:29, imeevma at tarantool.org wrote:
> > Part of #4145
> > ---
> > src/box/sql/func.c | 38 +++++++++++++++++++++++++++++++++++---
> > 1 file changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 54b03f359..2e53b32d8 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -263,6 +263,38 @@ func_abs_double(struct sql_context *ctx, int argc, struct Mem *argv)
> > mem_set_double(ctx->pOut, arg->u.r < 0 ? -arg->u.r : arg->u.r);
> > }
> >
> > +/** Implementation of the CHAR_LENGTH() function. */
> > +static inline uint8_t
> > +utf8_len_char(char c)
> > +{
> > + uint8_t u = (uint8_t)c;
> > + return 1 + (u >= 0xc2) + (u >= 0xe0) + (u >= 0xf0);
>
> It is not that simple really. Consider either using the old
> lengthFunc() and other sqlite utf8 helpers or use the approach
> similar to utf8_len() in utf8.c. It uses ICU macro U8_NEXT()
> and has handling for special symbols like U_SENTINEL.
>
> Otherwise you are making already third version of functions to
> work with utf8.
>
> I would even prefer to refactor lengthFunc() to stop using sqlite
> legacy and drop sqlite utf8 entirely, but I suspect it might be
> not so trivial to do and should be done later.
I was able to use ucnv_getNextUChar() here. In fact, I was able to use this
functions in all the places in this patch-set where we had to work with my or
SQLite functions that work with UTF8 characters. I think I can remove sql/utf.c
in the next patchset, since I refactor the LENGTH() and UNICODE() functions
there.
Diff:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index e7d1988e0..faef0eef3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -259,35 +259,24 @@ func_abs_double(struct sql_context *ctx, int argc, struct Mem *argv)
}
/** Implementation of the CHAR_LENGTH() function. */
-static inline uint8_t
-utf8_len_char(char c)
-{
- uint8_t u = (uint8_t)c;
- return 1 + (u >= 0xc2) + (u >= 0xe0) + (u >= 0xf0);
-}
-
-static inline uint32_t
-utf8_len_str(const char *str, uint32_t size)
-{
- uint32_t len = 0;
- uint32_t pos = 0;
- while (pos < size) {
- pos += utf8_len_char(str[pos]);
- ++len;
- }
- return len;
-}
-
static void
func_char_length(struct sql_context *ctx, int argc, struct Mem *argv)
{
assert(argc == 1);
(void)argc;
struct Mem *arg = &argv[0];
- if (arg->type == MEM_TYPE_NULL)
+ if (mem_is_null(arg))
return;
- assert(arg->type == MEM_TYPE_STR && arg->n >= 0);
- mem_set_uint(ctx->pOut, utf8_len_str(arg->z, arg->n));
+ assert(mem_is_str(arg) && arg->n >= 0);
+ uint32_t len = 0;
+ UErrorCode err = U_ZERO_ERROR;
+ const char *pos = arg->z;
+ const char *end = arg->z + arg->n;
+ while (pos < end) {
+ ucnv_getNextUChar(icu_utf8_conv, &pos, end, &err);
+ ++len;
+ }
+ mem_set_uint(ctx->pOut, len);
}
static const unsigned char *
diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua
index b25436186..27f17168b 100755
--- a/test/sql-tap/badutf1.test.lua
+++ b/test/sql-tap/badutf1.test.lua
@@ -287,7 +287,7 @@ test:do_test(
return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80') AS x")
end, {
-- <badutf-3.8>
- "X", 7
+ "X", 10
-- </badutf-3.8>
})
@@ -297,7 +297,7 @@ test:do_test(
return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff') AS x")
end, {
-- <badutf-3.9>
- "X", 7
+ "X", 10
-- </badutf-3.9>
})
diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua
index 6fae811dc..7fe987abc 100755
--- a/test/sql-tap/built-in-functions.test.lua
+++ b/test/sql-tap/built-in-functions.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
local test = require("sqltester")
-test:plan(52)
+test:plan(54)
--
-- Make sure that number of arguments check is checked properly for SQL built-in
@@ -545,4 +545,28 @@ test:do_test(
{name = "COLUMN_2", type = "scalar"},
})
+-- gh-4145: Make sure the character is now checked when calculating its length.
+
+-- Character with UTF-8 code F0808080 does not exist.
+test:do_execsql_test(
+ "builtins-4.1",
+ [[
+ SELECT CHAR_LENGTH(CAST(x'f0808080' AS STRING));
+ ]],
+ {
+ 4
+ }
+)
+
+-- Character with UTF-8 code F0908080 is '𐀀'.
+test:do_execsql_test(
+ "builtins-4.2",
+ [[
+ SELECT CHAR_LENGTH(CAST(x'f0908080' AS STRING));
+ ]],
+ {
+ 1
+ }
+)
+
test:finish_test()
diff --git a/test/sql-tap/func3.test.lua b/test/sql-tap/func3.test.lua
index 7f1d8d33c..6999fea67 100755
--- a/test/sql-tap/func3.test.lua
+++ b/test/sql-tap/func3.test.lua
@@ -297,8 +297,8 @@ suits[5] = {str = '\x61\xc0', len = 2}
suits[6] = {str = '\x61\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 12}
suits[7] = {str = '\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 11}
suits[8] = {str = '\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 10}
-suits[9] = {str = '\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80', len = 7}
-suits[10] = {str = '\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff', len = 7}
+suits[9] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\x80', len = 7}
+suits[10] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\xff', len = 7}
for k,v in pairs(suits) do
test:do_execsql_test(
New patch:
commit 080a23b1d84ce7749d887819d328e988ee7fe766
Author: Mergen Imeev <imeevma at gmail.com>
Date: Fri Oct 1 11:12:39 2021 +0300
sql: rework CHAR_LENGTH() function
The CHAR_LENGTH() and CHARACTER_LENGTH() functions now use ICU functions
to determine the length of a string. This gives us a better pricision
when determining the length.
Part of #4145
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index dbeb38bee..faef0eef3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -258,6 +258,27 @@ func_abs_double(struct sql_context *ctx, int argc, struct Mem *argv)
mem_set_double(ctx->pOut, arg->u.r < 0 ? -arg->u.r : arg->u.r);
}
+/** Implementation of the CHAR_LENGTH() function. */
+static void
+func_char_length(struct sql_context *ctx, int argc, struct Mem *argv)
+{
+ assert(argc == 1);
+ (void)argc;
+ struct Mem *arg = &argv[0];
+ if (mem_is_null(arg))
+ return;
+ assert(mem_is_str(arg) && arg->n >= 0);
+ uint32_t len = 0;
+ UErrorCode err = U_ZERO_ERROR;
+ const char *pos = arg->z;
+ const char *end = arg->z + arg->n;
+ while (pos < end) {
+ ucnv_getNextUChar(icu_utf8_conv, &pos, end, &err);
+ ++len;
+ }
+ mem_set_uint(ctx->pOut, len);
+}
+
static const unsigned char *
mem_as_ustr(struct Mem *mem)
{
@@ -1912,8 +1933,8 @@ static struct sql_func_definition definitions[] = {
{"AVG", 1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_INTEGER, step_avg, fin_avg},
{"AVG", 1, {FIELD_TYPE_DOUBLE}, FIELD_TYPE_DOUBLE, step_avg, fin_avg},
{"CHAR", -1, {FIELD_TYPE_INTEGER}, FIELD_TYPE_STRING, charFunc, NULL},
- {"CHAR_LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, lengthFunc,
- NULL},
+ {"CHAR_LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER,
+ func_char_length, NULL},
{"COALESCE", -1, {FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR, sql_builtin_stub,
NULL},
{"COUNT", 0, {}, FIELD_TYPE_INTEGER, step_count, fin_count},
@@ -1957,7 +1978,7 @@ static struct sql_func_definition definitions[] = {
{"LEAST", -1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, minmaxFunc, NULL},
{"LEAST", -1, {FIELD_TYPE_SCALAR}, FIELD_TYPE_SCALAR, minmaxFunc, NULL},
- {"LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, lengthFunc,
+ {"LENGTH", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_INTEGER, func_char_length,
NULL},
{"LENGTH", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_INTEGER, lengthFunc,
NULL},
diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua
index b25436186..27f17168b 100755
--- a/test/sql-tap/badutf1.test.lua
+++ b/test/sql-tap/badutf1.test.lua
@@ -287,7 +287,7 @@ test:do_test(
return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80') AS x")
end, {
-- <badutf-3.8>
- "X", 7
+ "X", 10
-- </badutf-3.8>
})
@@ -297,7 +297,7 @@ test:do_test(
return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff') AS x")
end, {
-- <badutf-3.9>
- "X", 7
+ "X", 10
-- </badutf-3.9>
})
diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua
index 6fae811dc..7fe987abc 100755
--- a/test/sql-tap/built-in-functions.test.lua
+++ b/test/sql-tap/built-in-functions.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
local test = require("sqltester")
-test:plan(52)
+test:plan(54)
--
-- Make sure that number of arguments check is checked properly for SQL built-in
@@ -545,4 +545,28 @@ test:do_test(
{name = "COLUMN_2", type = "scalar"},
})
+-- gh-4145: Make sure the character is now checked when calculating its length.
+
+-- Character with UTF-8 code F0808080 does not exist.
+test:do_execsql_test(
+ "builtins-4.1",
+ [[
+ SELECT CHAR_LENGTH(CAST(x'f0808080' AS STRING));
+ ]],
+ {
+ 4
+ }
+)
+
+-- Character with UTF-8 code F0908080 is '𐀀'.
+test:do_execsql_test(
+ "builtins-4.2",
+ [[
+ SELECT CHAR_LENGTH(CAST(x'f0908080' AS STRING));
+ ]],
+ {
+ 1
+ }
+)
+
test:finish_test()
diff --git a/test/sql-tap/func3.test.lua b/test/sql-tap/func3.test.lua
index 7f1d8d33c..6999fea67 100755
--- a/test/sql-tap/func3.test.lua
+++ b/test/sql-tap/func3.test.lua
@@ -297,8 +297,8 @@ suits[5] = {str = '\x61\xc0', len = 2}
suits[6] = {str = '\x61\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 12}
suits[7] = {str = '\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 11}
suits[8] = {str = '\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 10}
-suits[9] = {str = '\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80', len = 7}
-suits[10] = {str = '\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff', len = 7}
+suits[9] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\x80', len = 7}
+suits[10] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\xff', len = 7}
for k,v in pairs(suits) do
test:do_execsql_test(
More information about the Tarantool-patches
mailing list