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 DDC696EC40; Sat, 25 Sep 2021 14:42:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DDC696EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632570160; bh=r0kxhO+pxvQUShcU8bBbhjP6cDyhtxy03lZoeyGbAsQ=; 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=cP1weMTDV9X6TMkXD3g1dBE8pwNak3ohlSqEXhlE69wbWfH2rXtvvdBElnMVRyw6i jvVkDjERFZ2tsPj+iae2rR14ct/yhLKo3rIphA7YXZ1OrZntUZW2WHaAK7w8uOTvBv 2mWzIecMijqJisSXkE1FIcyJY0UVybq2JiG9mEAY= 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 D18336EC40 for ; Sat, 25 Sep 2021 14:42:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D18336EC40 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mU64k-0000Ii-1y; Sat, 25 Sep 2021 14:42:38 +0300 Date: Sat, 25 Sep 2021 14:42:36 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210925114236.GI290467@tarantool.org> References: <265692b0b90cd9ce0a5362b55e81f7a2d598ad19.1632220375.git.imeevma@gmail.com> <527aac58-f3f4-bae9-3057-187edee7d8d4@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <527aac58-f3f4-bae9-3057-187edee7d8d4@tarantool.org> X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD96A58C36AA2E99649B7184DC37D5878A1D72FF1E1C21FC301182A05F5380850408D9416F69244FB31DDCFACD09BD6E631DA5C732A489ABF9AB68D2F308CE39F50 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F5F08398AF01CA1FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BEEB0D1ADA650026EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2BF0ABB222CB1A4B397DCC91712066403CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B953A8A48A05D51F175ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A53D87F8FF5C2F62D667695AE1EC7F45C9D5F9AEE06220D4C0D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75BFC02AB3DF06BA5A410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F804B400765FFFF9898162B513B9E9A954B60FC94D0D25523E3640DFDE80F683B87F05C53B029F291D7E09C32AA3244C75E8A3BB873127794E6963C86F10341CD08D48398F32B4A6729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqHUEAOTxd4h6WJ8Wq2/n/w== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D7BFD7D890B872A83EC8826388A24522683D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 12/15] sql: rework GROUP_CONCAT() 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 answers, diff and new patch below. On Thu, Sep 23, 2021 at 12:49:52AM +0200, Vladislav Shpilevoy wrote: > Thanks for the patch! > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index f699aa927..001a8641c 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -213,6 +213,52 @@ fin_minmax(struct sql_context *ctx) > > mem_copy(ctx->pOut, ctx->pMem); > > } > > > > +/** Implementation of the GROUP_CONCAT() function. */ > > +static void > > +step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv) > > +{ > > + assert(argc == 1 || argc == 2); > > + (void)argc; > > + if (argv[0]->type == MEM_TYPE_NULL) > > + return; > > + assert(mem_is_str(argv[0]) || mem_is_bin(argv[0])); > > + if (ctx->pMem->type == MEM_TYPE_NULL) { > > + if (mem_copy_str(ctx->pMem, argv[0]->z, argv[0]->n) != 0) > > 1. What if the argument is zeroblob with no actual memory allocated yet? > There will be '', which is wrong. I fixed this and added a test. > > + ctx->is_aborted = true; > > + return; > > + } > > + const char *sep = NULL; > > + int sep_len = 0; > > + if (argc == 1) { > > + sep = ","; > > + sep_len = 1; > > + } else if (argv[1]->type == MEM_TYPE_NULL) { > > + sep = ""; > > + sep_len = 0; > > + } else { > > + assert(mem_is_same_type(argv[0], argv[0])); > > + sep = argv[1]->z; > > + sep_len = argv[1]->n; > > + } > > + if (sep_len > 0) { > > + if (mem_append(ctx->pMem, sep, sep_len) != 0) { > > 2. Will it work if sep_len == 0? If yes, then I would propose to > drop the len check here and call the append always. > Fixed. I moved this check to mem_append(). > > + ctx->is_aborted = true; > > + return; > > + } Diff: diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 182fb85be..28094e258 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -223,34 +223,43 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv) { assert(argc == 1 || argc == 2); (void)argc; - if (argv[0]->type == MEM_TYPE_NULL) + if (mem_is_null(argv[0])) return; assert(mem_is_str(argv[0]) || mem_is_bin(argv[0])); - if (ctx->pMem->type == MEM_TYPE_NULL) { - if (mem_copy_str(ctx->pMem, argv[0]->z, argv[0]->n) != 0) + if (mem_is_null(ctx->pMem)) { + if (mem_copy(ctx->pMem, argv[0]) != 0) ctx->is_aborted = true; return; } + assert(!mem_is_zerobin(ctx->pMem)); const char *sep = NULL; int sep_len = 0; if (argc == 1) { sep = ","; sep_len = 1; - } else if (argv[1]->type == MEM_TYPE_NULL) { + } else if (mem_is_null(argv[1])) { sep = ""; sep_len = 0; } else { - assert(mem_is_same_type(argv[0], argv[0])); + assert(mem_is_same_type(argv[0], argv[1])); sep = argv[1]->z; sep_len = argv[1]->n; } - if (sep_len > 0) { - if (mem_append(ctx->pMem, sep, sep_len) != 0) { - ctx->is_aborted = true; - return; - } + if (mem_append(ctx->pMem, sep, sep_len) != 0) { + ctx->is_aborted = true; + return; + } + uint32_t size; + char *str; + if (mem_is_zerobin(argv[0])) { + size = argv[0]->u.nZero; + str = sqlDbMallocRawNN(sql_get(), size); + memset(str, 0, size); + } else { + size = argv[0]->n; + str = argv[0]->z; } - if (mem_append(ctx->pMem, argv[0]->z, argv[0]->n) != 0) { + if (mem_append(ctx->pMem, str, size) != 0) { ctx->is_aborted = true; return; } diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua index 416f27d69..bd8a8fe78 100755 --- a/test/sql-tap/func.test.lua +++ b/test/sql-tap/func.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(14680) +test:plan(14681) --!./tcltestrunner.lua -- 2001 September 15 @@ -2142,11 +2142,14 @@ test:do_execsql_test( -- }) --- do_test func-24.3 { --- execsql { --- SELECT group_concat(t1,' ' || rowid || ' ') FROM tbl1 --- } --- } {{this 2 program 3 is 4 free 5 software}} +test:do_execsql_test( + "func-24.3", + [[ + SELECT group_concat(zeroblob(10)); + ]], { + '\0\0\0\0\0\0\0\0\0\0' + }) + test:do_execsql_test( "func-24.4", [[ New patch: commit 5b8563e9b884c86885a3a91fc608fb144afb69a0 Author: Mergen Imeev Date: Thu Sep 9 18:37:00 2021 +0300 sql: rework GROUP_CONCAT() This patch simplifies SQL built-in aggregate function GROUP_CONCAT(). Part of #4145 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 3708440e3..28094e258 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -217,6 +217,61 @@ fin_minmax(struct sql_context *ctx) mem_copy(ctx->pOut, ctx->pMem); } +/** Implementation of the GROUP_CONCAT() function. */ +static void +step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv) +{ + assert(argc == 1 || argc == 2); + (void)argc; + if (mem_is_null(argv[0])) + return; + assert(mem_is_str(argv[0]) || mem_is_bin(argv[0])); + if (mem_is_null(ctx->pMem)) { + if (mem_copy(ctx->pMem, argv[0]) != 0) + ctx->is_aborted = true; + return; + } + assert(!mem_is_zerobin(ctx->pMem)); + const char *sep = NULL; + int sep_len = 0; + if (argc == 1) { + sep = ","; + sep_len = 1; + } else if (mem_is_null(argv[1])) { + sep = ""; + sep_len = 0; + } else { + assert(mem_is_same_type(argv[0], argv[1])); + sep = argv[1]->z; + sep_len = argv[1]->n; + } + if (mem_append(ctx->pMem, sep, sep_len) != 0) { + ctx->is_aborted = true; + return; + } + uint32_t size; + char *str; + if (mem_is_zerobin(argv[0])) { + size = argv[0]->u.nZero; + str = sqlDbMallocRawNN(sql_get(), size); + memset(str, 0, size); + } else { + size = argv[0]->n; + str = argv[0]->z; + } + if (mem_append(ctx->pMem, str, size) != 0) { + ctx->is_aborted = true; + return; + } +} + +/** Finalizer for the GROUP_CONCAT() function. */ +static void +fin_group_concat(struct sql_context *ctx) +{ + mem_copy(ctx->pOut, ctx->pMem); +} + static const unsigned char * mem_as_ustr(struct Mem *mem) { @@ -1761,73 +1816,6 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv) } } -/* - * group_concat(EXPR, ?SEPARATOR?) - */ -static void -groupConcatStep(sql_context * context, int argc, sql_value ** argv) -{ - const char *zVal; - StrAccum *pAccum; - const char *zSep; - int nVal, nSep; - if (argc != 1 && argc != 2) { - diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, - "GROUP_CONCAT", "1 or 2", argc); - context->is_aborted = true; - return; - } - if (mem_is_null(argv[0])) - return; - pAccum = - (StrAccum *) sql_aggregate_context(context, sizeof(*pAccum)); - - if (pAccum) { - sql *db = sql_context_db_handle(context); - int firstTerm = pAccum->mxAlloc == 0; - pAccum->mxAlloc = db->aLimit[SQL_LIMIT_LENGTH]; - if (!firstTerm) { - if (argc == 2) { - zSep = mem_as_str0(argv[1]); - nSep = mem_len_unsafe(argv[1]); - } else { - zSep = ","; - nSep = 1; - } - if (zSep) - sqlStrAccumAppend(pAccum, zSep, nSep); - } - zVal = mem_as_str0(argv[0]); - nVal = mem_len_unsafe(argv[0]); - if (zVal) - sqlStrAccumAppend(pAccum, zVal, nVal); - } -} - -static void -groupConcatFinalize(sql_context * context) -{ - StrAccum *pAccum; - pAccum = sql_aggregate_context(context, 0); - if (pAccum) { - if (pAccum->accError == STRACCUM_TOOBIG) { - diag_set(ClientError, ER_SQL_EXECUTE, "string or binary"\ - "string is too big"); - context->is_aborted = true; - } else if (pAccum->accError == STRACCUM_NOMEM) { - context->is_aborted = true; - } else { - char *str = sqlStrAccumFinish(pAccum); - int len = pAccum->nChar; - assert(len >= 0); - if (context->func->def->returns == FIELD_TYPE_STRING) - mem_set_str_dynamic(context->pOut, str, len); - else - mem_set_bin_dynamic(context->pOut, str, len); - } - } -} - int sql_is_like_func(struct Expr *expr) { @@ -1995,13 +1983,13 @@ static struct sql_func_definition definitions[] = { NULL}, {"GROUP_CONCAT", 1, {FIELD_TYPE_STRING}, FIELD_TYPE_STRING, - groupConcatStep, groupConcatFinalize}, + step_group_concat, fin_group_concat}, {"GROUP_CONCAT", 2, {FIELD_TYPE_STRING, FIELD_TYPE_STRING}, - FIELD_TYPE_STRING, groupConcatStep, groupConcatFinalize}, + FIELD_TYPE_STRING, step_group_concat, fin_group_concat}, {"GROUP_CONCAT", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_VARBINARY, - groupConcatStep, groupConcatFinalize}, + step_group_concat, fin_group_concat}, {"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, FIELD_TYPE_VARBINARY}, - FIELD_TYPE_VARBINARY, groupConcatStep, groupConcatFinalize}, + FIELD_TYPE_VARBINARY, step_group_concat, fin_group_concat}, {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, NULL}, {"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, FIELD_TYPE_SCALAR, diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua index 416f27d69..bd8a8fe78 100755 --- a/test/sql-tap/func.test.lua +++ b/test/sql-tap/func.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool local test = require("sqltester") -test:plan(14680) +test:plan(14681) --!./tcltestrunner.lua -- 2001 September 15 @@ -2142,11 +2142,14 @@ test:do_execsql_test( -- }) --- do_test func-24.3 { --- execsql { --- SELECT group_concat(t1,' ' || rowid || ' ') FROM tbl1 --- } --- } {{this 2 program 3 is 4 free 5 software}} +test:do_execsql_test( + "func-24.3", + [[ + SELECT group_concat(zeroblob(10)); + ]], { + '\0\0\0\0\0\0\0\0\0\0' + }) + test:do_execsql_test( "func-24.4", [[