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 0EC4A6EC5B; Wed, 14 Apr 2021 01:46:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0EC4A6EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618353996; bh=hIt7KPCpbhKIJkqrzzVmyrGangXYwvKAeELM1f243nE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=AYR6QpXvkH60W7n/OAvZT+OZC4mbr55pe12Vy/DnZPL0EbqErsJaQvnttbpLBCziF 2kO3YuD6Suh/jq5jVOkN0sdgpXIgOCd8k/gUrdxlIip7pioVKz9kMRX0A58c9xVyP0 GTK9Q9Pl5Rf5CN85AicGt833XtfeHbfJlTiB6bZw= Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 9A1066EC5B for ; Wed, 14 Apr 2021 01:46:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9A1066EC5B Received: by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1lWRnl-0004z3-MR; Wed, 14 Apr 2021 01:46:34 +0300 Date: Wed, 14 Apr 2021 01:46:32 +0300 To: Vladislav Shpilevoy Message-ID: <20210413224632.GA61490@tarantool.org> References: <200616f9ee5708f14c565046e2e01eacbef2be62.1617984948.git.imeevma@gmail.com> <011884a7-5727-83c1-e2f3-aa7b849dafd4@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <011884a7-5727-83c1-e2f3-aa7b849dafd4@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480BE79914FF86F9151AC38CC435EA4A654182A05F538085040C4E75B934C745255CABD3818B0C6F79FC14401992039E86EB1940636D8A83082 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE795530B80AF2ADB7BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C8DFB935205A313D8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2301FE77B9136A498EEA962259216DFE23896EE2C32233946D2E47CDBA5A96583C09775C1D3CA48CF17B107DEF921CE79117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7ABB305BD10C6E5099FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE7FA3D786573799A4ED32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C0CEDA8D6C8C3B0531B3661434B16C20AC78D18283394535A9E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B62CFFCC7B69C47339089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824A2A04A2ABAA09D25379311020FFC8D4ADD69086D7A80F17D317E46EF1E6409770 X-C1DE0DAB: 0D63561A33F958A5903B8C24E062CAC6E15F239454391D8A24577DAF916D73A9D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34BC3EEE75EF3BACCF34FBAD8373B78FD7DE7E76662ADE717ABB93C02231774932E1F14F34DAD8C88B1D7E09C32AA3244C168E6E161373A68FF01432B093752EC4A95CA90A1D8AC565FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXEdl8uQv2eCFQ== X-Mailru-Sender: 5C3750E245F362008BC1685FEC6306EDABF2D3830836EEDDCABD3818B0C6F79FF1FEA02A07AA46D65105BD0848736F9966FEC6BF5C9C28D97E07721503EA2E00ED97202A5A4E92BF7402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 36/52] sql: introduce mem_set_agg() 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answer, diff and new patch below. On Tue, Apr 13, 2021 at 01:37:49AM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index b2598816d..af11ae1d5 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -612,6 +612,23 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame) > > mem->u.pFrame = frame; > > } > > > > +int > > +mem_set_agg(struct Mem *mem, struct func *func, int size) > > +{ > > + if (size <= 0) { > > + mem_clear(mem); > > + return 0; > > + } > > + if (sqlVdbeMemGrow(mem, size, 0) != 0) > > + return -1; > > + memset(mem->z, 0, size); > > + mem->n = size; > > + mem->flags = MEM_Agg; > > What if it already was MEM_Agg before? sqlVdbeMemGrow() > didn't clear the old value. It clears only MEM_Dyn. > You are right. Added mem_clear(). > > + mem->u.func = func; > > + mem->field_type = field_type_MAX; > > + return 0; > > +} Diff: diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index fdbd15b46..172883a44 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -472,10 +472,9 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame) int mem_set_agg(struct Mem *mem, struct func *func, int size) { - if (size <= 0) { - mem_clear(mem); + mem_clear(mem); + if (size <= 0) return 0; - } if (sqlVdbeMemGrow(mem, size, 0) != 0) return -1; memset(mem->z, 0, size); New patch: commit f8490d6176d02e144e1e3241b04c2c09f8390e78 Author: Mergen Imeev Date: Tue Mar 16 15:02:08 2021 +0300 sql: introduce mem_set_agg() This patch introduces mem_set_agg() function. This function stores aggregation function to MEM and allocates enough memory to hold accumulation structure for aggregate function. Part of #5818 diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 8e13131f1..172883a44 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -469,6 +469,22 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame) mem->u.pFrame = frame; } +int +mem_set_agg(struct Mem *mem, struct func *func, int size) +{ + mem_clear(mem); + if (size <= 0) + return 0; + if (sqlVdbeMemGrow(mem, size, 0) != 0) + return -1; + memset(mem->z, 0, size); + mem->n = size; + mem->flags = MEM_Agg; + mem->u.func = func; + mem->field_type = field_type_MAX; + return 0; +} + int mem_copy(struct Mem *to, const struct Mem *from) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 36b7d3eca..6dee83dc5 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -546,6 +546,13 @@ mem_set_ptr(struct Mem *mem, void *ptr); void mem_set_frame(struct Mem *mem, struct VdbeFrame *frame); +/** + * Clear the MEM, set the function as its value, and allocate enough memory to + * hold the accumulation structure for the aggregate function. + */ +int +mem_set_agg(struct Mem *mem, struct func *func, int size); + /** * Copy content of MEM from one MEM to another. In case source MEM contains * string or binary and allocation type is not STATIC, this value is copied to diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 65927910a..af1174d0a 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -383,29 +383,6 @@ sqlStmtCurrentTime(sql_context * p) return *piTime; } -/* - * Create a new aggregate context for p and return a pointer to - * its pMem->z element. - */ -static SQL_NOINLINE void * -createAggContext(sql_context * p, int nByte) -{ - Mem *pMem = p->pMem; - assert(!mem_is_agg(pMem)); - if (nByte <= 0) { - mem_set_null(pMem); - pMem->z = 0; - } else { - sqlVdbeMemClearAndResize(pMem, nByte); - pMem->flags = MEM_Agg; - pMem->u.func = p->func; - if (pMem->z) { - memset(pMem->z, 0, nByte); - } - } - return (void *)pMem->z; -} - /* * Allocate or return the aggregate context for a user function. A new * context is allocated on the first call. Subsequent calls return the @@ -417,12 +394,9 @@ sql_aggregate_context(sql_context * p, int nByte) assert(p != NULL && p->func != NULL); assert(p->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN); assert(p->func->def->aggregate == FUNC_AGGREGATE_GROUP); - testcase(nByte < 0); - if (!mem_is_agg(p->pMem)) { - return createAggContext(p, nByte); - } else { - return (void *)p->pMem->z; - } + if (!mem_is_agg(p->pMem) && mem_set_agg(p->pMem, p->func, nByte) != 0) + return NULL; + return p->pMem->z; } /*