[tarantool-patches] Re: [PATCH v1 1/1] sql: create CHAR_LENGTH() and CHARACTER_LENGTH()
Mergen Imeev
imeevma at tarantool.org
Sat Jun 15 17:45:51 MSK 2019
Hi! Thank you for review! I divided path into two. Both
patches and my answers below.
On Thu, Jun 06, 2019 at 09:55:31PM +0300, n.pettik wrote:
>
>
> > On 29 May 2019, at 20:57, Mergen Imeev <imeevma at tarantool.org> wrote:
> >
> > Hi! Thank you for review! My answers and new version below.
> >
> > On Tue, May 21, 2019 at 03:58:55PM +0300, n.pettik wrote:
> >>
> >>
> >>> On 8 May 2019, at 16:15, imeevma at tarantool.org wrote:
> >>>
> >>> This patch creates SQL functions CHARACTER_LENGTH() and
> >>> CHAR_LENGTH(). These functions work the same way. Each of them
> >>> takes only one argument of type TEXT and returns its length.
> >>
> >> I see your message in issue description:
> >>
> >> “
> >> About this function, it was decided that it would be better to
> >> implement it as "{CHAR_LENGTH | CHARACTER_LENGTH}
> >> (character value expression)" (without "[USING CHARACTERS]”)
> >> “
> >>
> >> Could you please provide explanation of this decision?
It seems that now is not the time for this.
> > Since this issue is not of primary importance, after some
> > discussion it was decided that it would be too troublesome to
> > create these functions with the support of “[USING CHARACTERS]”.
> >
> >> What then is the difference between length() and char_length()?
> >>
> > I think the most noticeable difference is that the CHAR_LENGTH and
> > CHARACTER_LENGTH functions are described in ANSI, but LENGTH is
> > not.
>
> So the name is the only difference? Why don’t you use lengthFunc then
> as an implementation? I guess the main purpose of the ticket not to add
> just another one synonym, but follow ANSI syntax of function invocation
> including USING clause.
>
Fixed by changes in lengthFunc.
> >>> diff --git a/test/sql-tap/func3.test.lua b/test/sql-tap/func3.test.lua
> >>> index 6d6411c..a3a548b 100755
> >>> --- a/test/sql-tap/func3.test.lua
> >>> +++ b/test/sql-tap/func3.test.lua
> >>> @@ -1,6 +1,6 @@
> >>> #!/usr/bin/env tarantool
> >>> test = require("sqltester")
> >>> -test:plan(25)
> >>> +test:plan(37)
> >>>
> >>> --!./tcltestrunner.lua
> >>> -- 2010 August 27
> >>> @@ -284,6 +284,134 @@ test:do_test(
> >>> return test:execsql "EXPLAIN SELECT likely(min(1.0+'2.0',4*11))"
> >>> end, test:execsql "EXPLAIN SELECT min(1.0+'2.0',4*11)”)
> >>
> >> Add test involving invalid utf8 characters.
> >>
> > Sorry, I could not understand what it means. Do you mean
> > something like this (not exactly invalid characters, but
> > still):
> > tarantool> box.execute("SELECT CHAR_LENGTH('\u{12345}');")
> > ---
> > - metadata:
> > - name: !!binary Q0hBUl9MRU5HVEgoJ/CSjYUnKQ==
> > type: integer
> > rows:
> > - [1]
> > ...
> >
> > I'm not sure if this should be added.
> > I added a test with utf8 characters.
>
> See sql-tap/badutf1.test.lua
>
Added.
First patch:
>From 3702718fcf9da52a27bebd5db67a3428b98a093d Mon Sep 17 00:00:00 2001
Date: Fri, 14 Jun 2019 18:55:51 +0300
Subject: [PATCH] sql: make LENGTH() to accept only strings
Prior to this patch, LENGTH() could accept arguments other than
strings. Since this function is considered to be the CHAR_LENGTH()
function from the ANSI standard, this is incorrect. This patch
corrects the behavior of the LENGTH() function.
Part of #3929
Part of #4159
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index ec4f76d..da820f5 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -138,31 +138,23 @@ typeofFunc(sql_context * context, int NotUsed, sql_value ** argv)
static void
lengthFunc(sql_context * context, int argc, sql_value ** argv)
{
- int len;
-
assert(argc == 1);
UNUSED_PARAMETER(argc);
- switch (sql_value_type(argv[0])) {
- case MP_BIN:
- case MP_INT:
- case MP_DOUBLE:{
- sql_result_int(context,
- sql_value_bytes(argv[0]));
- break;
- }
- case MP_STR:{
- const unsigned char *z = sql_value_text(argv[0]);
- if (z == 0)
- return;
- len = sql_utf8_char_count(z, sql_value_bytes(argv[0]));
- sql_result_int(context, len);
- break;
- }
- default:{
- sql_result_null(context);
- break;
- }
+ if (sql_value_type(argv[0]) == MP_NIL) {
+ sql_result_null(context);
+ return;
}
+ if (sql_value_type(argv[0]) != MP_STR) {
+ diag_set(ClientError, ER_INCONSISTENT_TYPES, "TEXT",
+ mem_type_to_str(argv[0]));
+ context->is_aborted = true;
+ return;
+ }
+ const unsigned char *z = sql_value_text(argv[0]);
+ if (z == NULL)
+ return;
+ int len = sql_utf8_char_count(z, sql_value_bytes(argv[0]));
+ sql_result_int(context, len);
}
/*
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index f9044ad..328e9b0 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(14590)
+test:plan(14589)
--!./tcltestrunner.lua
-- 2001 September 15
@@ -95,10 +95,10 @@ test:do_execsql_test(
test:do_execsql_test(
"func-1.4",
[[
- SELECT coalesce(length(a),-1) FROM t2
+ SELECT coalesce(a,-1) FROM t2
]], {
-- <func-1.4>
- 1, -1, 3, -1, 5
+ 1, -1, 345, -1, 67890
-- </func-1.4>
})
@@ -982,17 +982,6 @@ test:do_execsql_test(
-- </func-9.4>
})
-test:do_execsql_test(
- "func-9.5",
- [[
- SELECT length(randomblob(32)), length(randomblob(-5)),
- length(randomblob(2000))
- ]], {
- -- <func-9.5>
- 32, "", 2000
- -- </func-9.5>
- })
-
-- The "hex()" function was added in order to be able to render blobs
-- generated by randomblob(). So this seems like a good place to test
-- hex().
diff --git a/test/sql-tap/func3.test.lua b/test/sql-tap/func3.test.lua
index 6d6411c..3832cac 100755
--- a/test/sql-tap/func3.test.lua
+++ b/test/sql-tap/func3.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(25)
+test:plan(30)
--!./tcltestrunner.lua
-- 2010 August 27
@@ -284,6 +284,58 @@ test:do_test(
return test:execsql "EXPLAIN SELECT likely(min(1.0+'2.0',4*11))"
end, test:execsql "EXPLAIN SELECT min(1.0+'2.0',4*11)")
+ --
+-- gh-4159: sql: length takes not-string values.
+--
+test:do_catchsql_test(
+ "func3-6.1",
+ [[
+ SELECT LENGTH(12);
+ ]], {
+ -- <func3-6.1>
+ 1,"Inconsistent types: expected TEXT got INTEGER"
+ -- </func3-6.1>
+ })
+
+test:do_catchsql_test(
+ "func3-6.2",
+ [[
+ SELECT LENGTH(12.34);
+ ]], {
+ -- <func3-6.2>
+ 1,"Inconsistent types: expected TEXT got REAL"
+ -- </func3-6.2>
+ })
+
+test:do_catchsql_test(
+ "func3-6.3",
+ [[
+ SELECT LENGTH(x'12');
+ ]], {
+ -- <func3-6.3>
+ 1,"Inconsistent types: expected TEXT got BLOB"
+ -- </func3-6.3>
+ })
+
+test:do_catchsql_test(
+ "func3-6.4",
+ [[
+ SELECT LENGTH(true);
+ ]], {
+ -- <func3-6.4>
+ 1,"Inconsistent types: expected TEXT got BOOLEAN"
+ -- </func3-6.4>
+ })
+
+test:do_execsql_test(
+ "func3-6.5",
+ [[
+ SELECT LENGTH('123456789');
+ ]], {
+ -- <func3-6.5>
+ 9
+ -- </func3-6.5>
+ })
test:finish_test()
diff --git a/test/sql-tap/orderby1.test.lua b/test/sql-tap/orderby1.test.lua
index 51e8d30..2cbb8ef 100755
--- a/test/sql-tap/orderby1.test.lua
+++ b/test/sql-tap/orderby1.test.lua
@@ -735,7 +735,7 @@ test:do_execsql_test(
SELECT (
SELECT 'hardware' FROM (
SELECT 'software' ORDER BY 'firmware' ASC, 'sportswear' DESC
- ) GROUP BY 1 HAVING length(b) <> 0
+ ) GROUP BY 1 HAVING b <> 0
)
FROM abc;
]], {
Second patch:
>From 2db336d071f5ab2c1380961df6c0234d16af77d3 Mon Sep 17 00:00:00 2001
Date: Sat, 15 Jun 2019 13:51:58 +0300
Subject: [PATCH] sql: ANSI aliases for LENGTH().
This patch creates CHARACTER_LENGTH() and CHAR_LENGTH() aliases
for LENGTH().
Closes #3929
@TarantoolBot document
Title: SQL functions LENGTH(), CHAR_LENGTH() and CHARACTER_LENGTH()
The SQL functions LENGTH(), CHAR_LENGTH() and CHARACTER_LENGTH()
take exactly one argument. If the argument is of type TEXT, then
they return its length. If the argument is NULL, they return NULL.
They throw an error if the argument is of a different type or if
more than one argument is given.
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index da820f5..21e86bb 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1891,6 +1891,9 @@ sqlRegisterBuiltinFunctions(void)
FIELD_TYPE_STRING),
FUNCTION2(length, 1, 0, 0, lengthFunc, SQL_FUNC_LENGTH,
FIELD_TYPE_INTEGER),
+ FUNCTION(char_length, 1, 0, 0, lengthFunc, FIELD_TYPE_INTEGER),
+ FUNCTION(character_length, 1, 0, 0, lengthFunc,
+ FIELD_TYPE_INTEGER),
FUNCTION(position, 2, 0, 1, position_func, FIELD_TYPE_INTEGER),
FUNCTION(printf, -1, 0, 0, printfFunc, FIELD_TYPE_STRING),
FUNCTION(unicode, 1, 0, 0, unicodeFunc, FIELD_TYPE_STRING),
diff --git a/test/sql-tap/func3.test.lua b/test/sql-tap/func3.test.lua
index 3832cac..f5f5bba 100755
--- a/test/sql-tap/func3.test.lua
+++ b/test/sql-tap/func3.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(30)
+test:plan(58)
--!./tcltestrunner.lua
-- 2010 August 27
@@ -337,5 +337,269 @@ test:do_execsql_test(
-- </func3-6.5>
})
+ --
+-- gh-3929: sql: ANSI aliases for LENGTH().
+--
+test:do_catchsql_test(
+ "func3-7.1",
+ [[
+ SELECT CHAR_LENGTH(12);
+ ]], {
+ -- <func3-7.1>
+ 1,"Inconsistent types: expected TEXT got INTEGER"
+ -- </func3-7.1>
+ })
+
+test:do_catchsql_test(
+ "func3-7.2",
+ [[
+ SELECT CHAR_LENGTH(12.34);
+ ]], {
+ -- <func3-7.2>
+ 1,"Inconsistent types: expected TEXT got REAL"
+ -- </func3-7.2>
+ })
+
+test:do_catchsql_test(
+ "func3-7.3",
+ [[
+ SELECT CHAR_LENGTH(x'12');
+ ]], {
+ -- <func3-7.3>
+ 1,"Inconsistent types: expected TEXT got BLOB"
+ -- </func3-7.3>
+ })
+
+test:do_catchsql_test(
+ "func3-7.4",
+ [[
+ SELECT CHAR_LENGTH(true);
+ ]], {
+ -- <func3-7.4>
+ 1,"Inconsistent types: expected TEXT got BOOLEAN"
+ -- </func3-7.4>
+ })
+
+test:do_execsql_test(
+ "func3-7.5",
+ [[
+ SELECT CHAR_LENGTH('123456789');
+ ]], {
+ -- <func3-7.5>
+ 9
+ -- </func3-7.5>
+ })
+
+test:do_execsql_test(
+ "func3-7.6",
+ "SELECT CHAR_LENGTH('\x80');",
+ {
+ -- <func3-7.6>
+ 1
+ -- </func3-7.6>
+ })
+
+test:do_execsql_test(
+ "func3-7.7",
+ "SELECT CHAR_LENGTH('\x61\x62\x63');",
+ {
+ -- <func3-7.7>
+ 3
+ -- </func3-7.7>
+ })
+
+test:do_execsql_test(
+ "func3-7.8",
+ "SELECT CHAR_LENGTH('\x7f\x80\x81');",
+ {
+ -- <func3-7.8>
+ 3
+ -- </func3-7.8>
+ })
+
+test:do_execsql_test(
+ "func3-7.9",
+ "SELECT CHAR_LENGTH('\x61\xc0');",
+ {
+ -- <func3-7.9>
+ 2
+ -- </func3-7.9>
+ })
+
+test:do_execsql_test(
+ "func3-7.10",
+ "SELECT CHAR_LENGTH('\x61\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80');",
+ {
+ -- <func3-7.10>
+ 12
+ -- </func3-7.10>
+ })
+
+test:do_execsql_test(
+ "func3-7.11",
+ "SELECT CHAR_LENGTH('\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80');",
+ {
+ -- <func3-7.11>
+ 11
+ -- </func3-7.11>
+ })
+
+test:do_execsql_test(
+ "func3-7.12",
+ "SELECT CHAR_LENGTH('\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80');",
+ {
+ -- <func3-7.12>
+ 10
+ -- </func3-7.12>
+ })
+
+test:do_execsql_test(
+ "func3-7.13",
+ "SELECT CHAR_LENGTH('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80');",
+ {
+ -- <func3-7.13>
+ 7
+ -- </func3-7.13>
+ })
+
+test:do_execsql_test(
+ "func3-7.14",
+ "SELECT CHAR_LENGTH('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff');",
+ {
+ -- <func3-7.14>
+ 7
+ -- </func3-7.14>
+ })
+
+test:do_catchsql_test(
+ "func3-7.15",
+ [[
+ SELECT CHARACTER_LENGTH(12);
+ ]], {
+ -- <func3-7.15>
+ 1,"Inconsistent types: expected TEXT got INTEGER"
+ -- </func3-7.15>
+ })
+
+test:do_catchsql_test(
+ "func3-7.16",
+ [[
+ SELECT CHARACTER_LENGTH(12.34);
+ ]], {
+ -- <func3-7.16>
+ 1,"Inconsistent types: expected TEXT got REAL"
+ -- </func3-7.16>
+ })
+
+test:do_catchsql_test(
+ "func3-7.17",
+ [[
+ SELECT CHARACTER_LENGTH(x'12');
+ ]], {
+ -- <func3-7.17>
+ 1,"Inconsistent types: expected TEXT got BLOB"
+ -- </func3-7.17>
+ })
+
+test:do_catchsql_test(
+ "func3-7.18",
+ [[
+ SELECT CHARACTER_LENGTH(true);
+ ]], {
+ -- <func3-7.18>
+ 1,"Inconsistent types: expected TEXT got BOOLEAN"
+ -- </func3-7.18>
+ })
+
+test:do_execsql_test(
+ "func3-7.19",
+ [[
+ SELECT CHARACTER_LENGTH('123456789');
+ ]], {
+ -- <func3-7.19>
+ 9
+ -- </func3-7.19>
+ })
+
+test:do_execsql_test(
+ "func3-7.20",
+ "SELECT CHARACTER_LENGTH('\x80');",
+ {
+ -- <func3-7.20>
+ 1
+ -- </func3-7.20>
+ })
+
+test:do_execsql_test(
+ "func3-7.21",
+ "SELECT CHARACTER_LENGTH('\x61\x62\x63');",
+ {
+ -- <func3-7.21>
+ 3
+ -- </func3-7.21>
+ })
+
+test:do_execsql_test(
+ "func3-7.22",
+ "SELECT CHARACTER_LENGTH('\x7f\x80\x81');",
+ {
+ -- <func3-7.22>
+ 3
+ -- </func3-7.22>
+ })
+
+test:do_execsql_test(
+ "func3-7.23",
+ "SELECT CHARACTER_LENGTH('\x61\xc0');",
+ {
+ -- <func3-7.23>
+ 2
+ -- </func3-7.23>
+ })
+
+test:do_execsql_test(
+ "func3-7.24",
+ "SELECT CHARACTER_LENGTH('\x61\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80');",
+ {
+ -- <func3-7.24>
+ 12
+ -- </func3-7.24>
+ })
+
+test:do_execsql_test(
+ "func3-7.25",
+ "SELECT CHARACTER_LENGTH('\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80');",
+ {
+ -- <func3-7.25>
+ 11
+ -- </func3-7.25>
+ })
+
+test:do_execsql_test(
+ "func3-7.26",
+ "SELECT CHARACTER_LENGTH('\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80');",
+ {
+ -- <func3-7.26>
+ 10
+ -- </func3-7.26>
+ })
+
+test:do_execsql_test(
+ "func3-7.27",
+ "SELECT CHARACTER_LENGTH('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80');",
+ {
+ -- <func3-7.27>
+ 7
+ -- </func3-7.27>
+ })
+
+test:do_execsql_test(
+ "func3-7.28",
+ "SELECT CHARACTER_LENGTH('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff');",
+ {
+ -- <func3-7.28>
+ 7
+ -- </func3-7.28>
+ })
test:finish_test()
More information about the Tarantool-patches
mailing list