From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 7723E6ECE3; Mon, 1 Nov 2021 13:20:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7723E6ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1635762034; bh=tCCGy33Ygg6jko0E3+DErSuQpgU3nzqXpDfmbQCXyVw=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=XaxrP5R1elECetpf1GO33O56bHflrOMKiO3kEtwmKlGrv6Ohinlm9eRbW2DzrfFWX pRe5p3v/Gv++DvY6xjcOST/raS67d5PvgimAinOItf+LZiB4xPjqoRteaZYB6ElQLR qyH3g+9vWv3yOmIP84JlCHTAb3JP37wGdjCEUyMk= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EA5B16ECE3 for ; Mon, 1 Nov 2021 13:20:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EA5B16ECE3 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mhUQa-0001m5-2k; Mon, 01 Nov 2021 13:20:32 +0300 Date: Mon, 1 Nov 2021 13:20:30 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20211101102030.GB227579@tarantool.org> References: <9cc35ba4625d4e3017725c35fbc4a7ed90341917.1633105483.git.imeevma@gmail.com> <20211020165844.GB203963@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD98893113B6235BE919ECA157A254E6E778771CCD3CA08B4A8182A05F53808504029667FB59CAFBB34C2EEABA4BAFD9893C75C2363EB32E560E44D1C13EF41B33C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7059B0D8AC717918AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375DA0115FC9FAE6C68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87140555D2EDECF26FE376B06E33BDC10117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66F6A3E018CF4DC80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5A9A0AD0F1257E6D7302E98D3598F8A9B8170F4CCB7C8C108D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D348F9E8EBB46231487C48EBE88EE41F1C02E773AB664979335DAEE84844DD2DC92FAC76E7A570D28A31D7E09C32AA3244CC86A8A941807588740175688CAEFB836259227199D06760A729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsm8N/O5xSHRF7BjeTax+fQ== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DC172C3EA206599F3BB5ED6ACAB6E89DC83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 2/8] sql: refactor CHAR_LENGTH() function X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answer, diff and new patch below. I also dropped some tests, including the ones I added in the previous version of the patch. I did this because the behavior of the ICU functions caused some of the CI targets to crash. I added a doc-bot request for this issue in the commit message. On Fri, Oct 29, 2021 at 12:11:37AM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > >>> +/** 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. > > Discussed in private that U8_NEXT() would work here just fine. > ucnv_getNextUChar() is an overkill. In other places of the patchset too. > Thank you for the suggestion! I replaces ucnv_getNextUChar() by U8_NEXT(). Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index faef0eef3..bc7a1fedd 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -269,11 +269,10 @@ func_char_length(struct sql_context *ctx, int argc, struct Mem *argv) 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); + int offset = 0; + while (offset < arg->n) { + UChar32 c; + U8_NEXT((uint8_t *)arg->z, offset, arg->n, c); ++len; } mem_set_uint(ctx->pOut, len); diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua index 27f17168b..ce8354840 100755 --- a/test/sql-tap/badutf1.test.lua +++ b/test/sql-tap/badutf1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(23) +test:plan(19) --!./tcltestrunner.lua -- 2007 May 15 @@ -253,26 +253,6 @@ test:do_test( test:do_test( "badutf-3.5", - function() - return test:execsql2("SELECT length('\x61\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80') AS x") - end, { - -- - "X", 12 - -- - }) - -test:do_test( - "badutf-3.6", - function() - return test:execsql2("SELECT length('\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80') AS x") - end, { - -- - "X", 11 - -- - }) - -test:do_test( - "badutf-3.7", function() return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80') AS x") end, { @@ -281,26 +261,6 @@ test:do_test( -- }) -test:do_test( - "badutf-3.8", - function() - return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80') AS x") - end, { - -- - "X", 10 - -- - }) - -test:do_test( - "badutf-3.9", - function() - return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff') AS x") - end, { - -- - "X", 10 - -- - }) - test:do_test( "badutf-4.1", function() diff --git a/test/sql-tap/built-in-functions.test.lua b/test/sql-tap/built-in-functions.test.lua index 7fe987abc..6fae811dc 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(54) +test:plan(52) -- -- Make sure that number of arguments check is checked properly for SQL built-in @@ -545,28 +545,4 @@ 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 6999fea67..7ed0bb27f 100755 --- a/test/sql-tap/func3.test.lua +++ b/test/sql-tap/func3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(35) +test:plan(33) --!./tcltestrunner.lua -- 2010 August 27 @@ -294,11 +294,9 @@ suits[2] = {str = '\x80', len = 1} suits[3] = {str = '\x61\x62\x63', len = 3} suits[4] = {str = '\x7f\x80\x81', len = 3} 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\x90\x80\x80\x80', len = 7} -suits[10] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\xff', len = 7} +suits[6] = {str = '\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 10} +suits[7] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\x80', len = 7} +suits[8] = {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 918fd18760f4491b81266b279dc5c4b581dc1ed6 Author: Mergen Imeev 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. Part of #4145 @TarantoolBot document Title: Invalid UTF-8 values and ICU Invalid UTF-8 values may be handled differently depending on the ICU version. For example, for this request: ``` SELECT CHAR_LENGTH(CAST(x'f0808080' AS STRING)); ``` On `centos 7` with `libicu-devel-50.2-4.el7_7.x86_64` the result will be: ``` tarantool> box.execute([[SELECT CHAR_LENGTH(CAST(x'f0808080' AS STRING));]]) --- - metadata: - name: COLUMN_1 type: integer rows: - [1] ... ``` On `ubuntu 20.04` with `libicu-dev` version `66.1-2ubuntu2` the result will be: ``` tarantool> box.execute([[SELECT CHAR_LENGTH(CAST(x'f0808080' AS STRING));]]) --- - metadata: - name: COLUMN_1 type: integer rows: - [4] ... ``` diff --git a/src/box/sql/func.c b/src/box/sql/func.c index dbeb38bee..bc7a1fedd 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -258,6 +258,26 @@ 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; + int offset = 0; + while (offset < arg->n) { + UChar32 c; + U8_NEXT((uint8_t *)arg->z, offset, arg->n, c); + ++len; + } + mem_set_uint(ctx->pOut, len); +} + static const unsigned char * mem_as_ustr(struct Mem *mem) { @@ -1912,8 +1932,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 +1977,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..ce8354840 100755 --- a/test/sql-tap/badutf1.test.lua +++ b/test/sql-tap/badutf1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(23) +test:plan(19) --!./tcltestrunner.lua -- 2007 May 15 @@ -253,26 +253,6 @@ test:do_test( test:do_test( "badutf-3.5", - function() - return test:execsql2("SELECT length('\x61\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80') AS x") - end, { - -- - "X", 12 - -- - }) - -test:do_test( - "badutf-3.6", - function() - return test:execsql2("SELECT length('\xc0\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80') AS x") - end, { - -- - "X", 11 - -- - }) - -test:do_test( - "badutf-3.7", function() return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80') AS x") end, { @@ -281,26 +261,6 @@ test:do_test( -- }) -test:do_test( - "badutf-3.8", - function() - return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\x80') AS x") - end, { - -- - "X", 7 - -- - }) - -test:do_test( - "badutf-3.9", - function() - return test:execsql2("SELECT length('\x80\x80\x80\x80\x80\xf0\x80\x80\x80\xff') AS x") - end, { - -- - "X", 7 - -- - }) - test:do_test( "badutf-4.1", function() diff --git a/test/sql-tap/func3.test.lua b/test/sql-tap/func3.test.lua index 7f1d8d33c..7ed0bb27f 100755 --- a/test/sql-tap/func3.test.lua +++ b/test/sql-tap/func3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(35) +test:plan(33) --!./tcltestrunner.lua -- 2010 August 27 @@ -294,11 +294,9 @@ suits[2] = {str = '\x80', len = 1} suits[3] = {str = '\x61\x62\x63', len = 3} suits[4] = {str = '\x7f\x80\x81', len = 3} 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[6] = {str = '\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80', len = 10} +suits[7] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\x80', len = 7} +suits[8] = {str = '\x80\x80\x80\x80\x80\xf0\x90\x80\x80\xff', len = 7} for k,v in pairs(suits) do test:do_execsql_test(