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 BD4286FC87; Wed, 29 Sep 2021 10:03:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BD4286FC87 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632898997; bh=ieje7u45ZUp1bUcoYCTp0AL4QXEq1dPLG1UrY+n9DYQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=m3lSzHWGEuLRP9YeeeGv8zU2PJ/XxKG/J7DbOeB5dFdh9D6+kd1Z7QXCn/AxkUUMo CjI5h0s6quhUvUinP4lMCJ3htMZXQh2W835kPBgC1qdDziCwzDlaBDAxInGh/Iuqt7 UyWcMrUFNtxKwIOb90mTsXilEx+OhH4TxOJ6ESvo= 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 7F3C06FC87 for ; Wed, 29 Sep 2021 10:03:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7F3C06FC87 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mVTcZ-0004fh-Lv; Wed, 29 Sep 2021 10:03:16 +0300 Date: Wed, 29 Sep 2021 10:03:14 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Message-ID: <20210929070314.GA88551@tarantool.org> References: <265692b0b90cd9ce0a5362b55e81f7a2d598ad19.1632220375.git.imeevma@gmail.com> <527aac58-f3f4-bae9-3057-187edee7d8d4@tarantool.org> <20210925114236.GI290467@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210925114236.GI290467@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96A58C36AA2E9964976B67DC8394C8DAB0EDC1900E6C7EF8D182A05F538085040314ACBF51ACAFA0C87E9DA142D0A9AD72A2DB1C97B0E8447BAB6833395D7563F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7850A3F981F25E362EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E8DF562B6063DE0A8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BF020C36EFA6F3D265EA8EC1FEA0FDD8117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66F6A3E018CF4DC80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A582ABA70659CABA5CB6FCFB2923E09A1898D994A04043147ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75BFC02AB3DF06BA5A410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3498910055B812BD9C3491750E79294F155F22CA5BBDE82BE35F5C5441998AF6DF91A163AEEF3D8C5F1D7E09C32AA3244C5D590C033259FF8AE910C2777A1620FC60759606DA2E136A729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojWaDhU1ub98Y/7ujbKmKHBg== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D47E2C9BBC79578CA259A1972B4CEA00A83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B 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" I found out that I forgot to free allocated memory in zeroblob case. Fixed. Diff in this patch and changed diffs in "sql: remove copying of result in finalizers" and "sql: remove field argv from struct sql_context" below. Also I rebased this branch to current master. On Sat, Sep 25, 2021 at 02:42:36PM +0300, Mergen Imeev via Tarantool-patches wrote: > 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", > [[ Changes in "sql: rework GROUP_CONCAT()": diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 28094e258..e1566e150 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -259,10 +259,10 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv) size = argv[0]->n; str = argv[0]->z; } - if (mem_append(ctx->pMem, str, size) != 0) { + if (mem_append(ctx->pMem, str, size) != 0) ctx->is_aborted = true; - return; - } + if (mem_is_zerobin(argv[0])) + sqlDbFree(sql_get(), str); } /** Finalizer for the GROUP_CONCAT() function. */ Changed diff in "sql: remove copying of result in finalizers": diff --git a/src/box/sql/func.c b/src/box/sql/func.c index e1566e150..cd256eef2 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -259,19 +244,12 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv) size = argv[0]->n; str = argv[0]->z; } - if (mem_append(ctx->pMem, str, size) != 0) + if (mem_append(ctx->pOut, str, size) != 0) ctx->is_aborted = true; if (mem_is_zerobin(argv[0])) sqlDbFree(sql_get(), str); } -/** 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) { Changed diff in "sql: remove field argv from struct sql_context": diff --git a/src/box/sql/func.c b/src/box/sql/func.c index cd256eef2..41b3de89b 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -236,17 +236,17 @@ step_group_concat(struct sql_context *ctx, int argc, struct Mem **argv) } uint32_t size; char *str; - if (mem_is_zerobin(argv[0])) { - size = argv[0]->u.nZero; + 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; + size = argv[0].n; + str = argv[0].z; } if (mem_append(ctx->pOut, str, size) != 0) ctx->is_aborted = true; - if (mem_is_zerobin(argv[0])) + if (mem_is_zerobin(&argv[0])) sqlDbFree(sql_get(), str); }