[tarantool-patches] Re: [PATCH v1 1/1] sql: create CHAR_LENGTH() and CHARACTER_LENGTH()

[Mergen Imeev <imeevma@tarantool.org>]

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

> I see that char_length() in your implementation doesn’t tolerate
> NULLs. Is it OK? Could you check this fact using ANSI spec?
> 
You are right:
"General Rules
1) If the value of any <character primary> or <blob primary>
simply contained in a <string value expression> is the null value,
then the result of the <string value expression> is the null
value."
I fixed this.

> > /*
> > + * Implementation of the char_length() and character_length()
> > + * functions.
> > + */
> 
> Nit:
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 01c271cab..b5fdc6f20 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -132,7 +132,7 @@ typeofFunc(sql_context * context, int NotUsed, sql_value ** argv)
>         sql_result_text(context, z, -1, SQL_STATIC);
>  }
>  
> -/*
> +/**
>   * Implementation of the char_length() and character_length()
>   * functions.
>   */
> 
> 
Thanks, fixed.

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


New patch:

From 150042175efa4e84b420fc90e02c5b70c4151d36 Mon Sep 17 00:00:00 2001
Date: Tue, 7 May 2019 20:33:39 +0300
Subject: [PATCH] sql: create CHAR_LENGTH() and CHARACTER_LENGTH()

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.

Closes #3929

@TarantoolBot document
Title: SQL functions CHAR_LENGTH() and CHARACTER_LENGTH()

The SQL functions CHAR_LENGTH() and CHARACTER_LENGTH() take
exactly one argument of type TEXT and return its length. 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 bb7405e..210a1de 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -132,6 +132,33 @@ typeofFunc(sql_context * context, int NotUsed, sql_value ** argv)
 	sql_result_text(context, z, -1, SQL_STATIC);
 }
 
+/**
+ * Implementation of the char_length() and character_length()
+ * functions.
+ */
+static void
+char_length_func(sql_context *context, int argc, sql_value **argv)
+{
+	assert(argc == 1);
+	UNUSED_PARAMETER(argc);
+	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->isError = SQL_TARANTOOL_ERROR;
+		context->fErrorOrAux = 1;
+		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);
+}
+
 /*
  * Implementation of the length() function
  */
@@ -1936,6 +1963,10 @@ sqlRegisterBuiltinFunctions(void)
 			  FIELD_TYPE_STRING),
 		FUNCTION2(length, 1, 0, 0, lengthFunc, SQL_FUNC_LENGTH,
 			  FIELD_TYPE_INTEGER),
+		FUNCTION(char_length, 1, 0, 0, char_length_func,
+			 FIELD_TYPE_INTEGER),
+		FUNCTION(character_length, 1, 0, 0, char_length_func,
+			 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 6d6411c..20f0aa7 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(40)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -284,6 +284,167 @@ 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-3929: create SQL functions CHARACTER_LENGTH() and
+-- CHAR_LENGTH().
+--
+test:do_test(
+	"func3-6.1",
+	function()
+		local some_text = 'some text'
+		return test:execsql(string.format([[
+			SELECT LENGTH('%s'), CHAR_LENGTH('%s'), CHARACTER_LENGTH('%s');
+		]], some_text, some_text, some_text))
+	end, {
+		-- <func3-6.1>
+		9, 9, 9
+		-- </func3-6.1>
+	})
+
+test:do_test(
+	"func3-6.2",
+	function()
+		local some_text = 'какой-то текст'
+		return test:execsql(string.format([[
+			SELECT LENGTH('%s'), CHAR_LENGTH('%s'), CHARACTER_LENGTH('%s');
+		]], some_text, some_text, some_text))
+	end, {
+		-- <func3-6.2>
+		14, 14, 14
+		-- </func3-6.2>
+	})
+
+test:do_catchsql_test(
+	"func3-6.3",
+	[[
+		SELECT CHAR_LENGTH(12);
+	]], {
+		-- <func3-6.3>
+		1,"Inconsistent types: expected TEXT got INTEGER"
+		-- </func3-6.3>
+	})
+
+test:do_catchsql_test(
+	"func3-6.4",
+	[[
+		SELECT CHAR_LENGTH(12.34);
+	]], {
+		-- <func3-6.4>
+		1,"Inconsistent types: expected TEXT got REAL"
+		-- </func3-6.4>
+	})
+
+test:do_catchsql_test(
+	"func3-6.5",
+	[[
+		SELECT CHAR_LENGTH(x'12');
+	]], {
+		-- <func3-6.5>
+		1,"Inconsistent types: expected TEXT got BLOB"
+		-- </func3-6.5>
+	})
+
+test:do_catchsql_test(
+	"func3-6.6",
+	[[
+		SELECT CHAR_LENGTH(true);
+	]], {
+		-- <func3-6.6>
+		1,"Inconsistent types: expected TEXT got BOOLEAN"
+		-- </func3-6.6>
+	})
+
+test:do_catchsql_test(
+	"func3-6.7",
+	[[
+		SELECT CHARACTER_LENGTH(12);
+	]], {
+		-- <func3-6.7>
+		1,"Inconsistent types: expected TEXT got INTEGER"
+		-- </func3-6.7>
+	})
 
+test:do_catchsql_test(
+	"func3-6.8",
+	[[
+		SELECT CHARACTER_LENGTH(12.34);
+	]], {
+		-- <func3-6.8>
+		1,"Inconsistent types: expected TEXT got REAL"
+		-- </func3-6.8>
+	})
+
+test:do_catchsql_test(
+	"func3-6.9",
+	[[
+		SELECT CHARACTER_LENGTH(x'12');
+	]], {
+		-- <func3-6.9>
+		1,"Inconsistent types: expected TEXT got BLOB"
+		-- </func3-6.9>
+	})
+
+test:do_catchsql_test(
+	"func3-6.10",
+	[[
+		SELECT CHARACTER_LENGTH(true);
+	]], {
+		-- <func3-6.10>
+		1,"Inconsistent types: expected TEXT got BOOLEAN"
+		-- </func3-6.10>
+	})
+
+test:do_catchsql_test(
+	"func3-6.11",
+	[[
+		SELECT CHAR_LENGTH('abc', 'efg');
+	]], {
+		-- <func3-6.11>
+		1,"wrong number of arguments to function CHAR_LENGTH()"
+		-- </func3-6.11>
+	})
+
+test:do_catchsql_test(
+	"func3-6.12",
+	[[
+		SELECT CHARACTER_LENGTH('abc', 'efg');
+	]], {
+		-- <func3-6.12>
+		1,"wrong number of arguments to function CHARACTER_LENGTH()"
+		-- </func3-6.12>
+	})
+
+test:do_execsql_test(
+	"func3-6.13",
+	[[
+		SELECT CHAR_LENGTH(NULL);
+	]], {
+		-- <func3-6.13>
+		""
+		-- </func3-6.13>
+	})
+
+test:do_execsql_test(
+	"func3-6.14",
+	[[
+		SELECT CHARACTER_LENGTH(NULL);
+	]], {
+		-- <func3-6.14>
+		""
+		-- </func3-6.14>
+	})
+
+test:do_test(
+	"func3-6.15",
+	function()
+		local some_text = '\u{123} \u{1234}'
+		return test:execsql(string.format([[
+			SELECT '%s', LENGTH('%s'), CHAR_LENGTH('%s'), CHARACTER_LENGTH('%s');
+		]], some_text, some_text, some_text, some_text))
+	end, {
+		-- <func3-6.15>
+		"ģ ሴ", 3, 3, 3
+		-- </func3-6.15>
+	})
 
 test:finish_test()