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 446456EC5F; Fri, 9 Apr 2021 23:25:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 446456EC5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617999953; bh=IOeoTXGjx/oUKFQcn/mSfgP0TjMsoLdGWDpG6M9PT5g=; h=To:Cc:Date:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=avu45ly0EjnCOA/5i8f3pxOXTpElhY319aECYgWvTcNyT13/FUG2z7xoujJO9HWix uSfjzXN2yeRxKTHmC8v/RossmGQzRck/6SIxnbPPv/H4CoIjW3ZOoWLT1vMocUrM9v jzUy1qiK5tm+hjB/BqPvmOYwuhes8byr/XwviOfc= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 E38C66EC5E for ; Fri, 9 Apr 2021 23:25:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E38C66EC5E Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lUxhP-000539-SS; Fri, 09 Apr 2021 23:25:52 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Date: Fri, 9 Apr 2021 23:25:51 +0300 Message-Id: <200616f9ee5708f14c565046e2e01eacbef2be62.1617984948.git.imeevma@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D5E28B957962BB550182A05F5380850402128F8D0C47B91FD73ABC9B969525C0B3190F87643D7EFACF3F87EF25129CC03 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B114C2C2C20B7E62EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373745FD4183B699148638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B239D0DE1B0E1A0B87730E983FC196265CB10649542A97EE81D2E47CDBA5A96583C09775C1D3CA48CFCA5A41EBD8A3A0199FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C327ED053E960B195E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BDC0F6C5B2EEF3D0C75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831C18EEF89901168EC916D7973594ED144 X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975CD0035DD76F8A8A4F2D89C9020C12A737BDEA89105E9A68A79C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D16EA493CC1FD9F8F005E12C0F6C24688BC247649487A8DB7B75758BD84492E82010D8A5FFEE53CF1D7E09C32AA3244C499685BA04B7F2508E3F159879C72D13FE8DA44ABE2443F7FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyO2lHpuZu4QbCyNjyYb3Hg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822E24D0FBA552C30CFF9B80EA356EF79BE83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: [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: imeevma@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answers and new patch below. On 30.03.2021 02:06, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 2 comments below. > >> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c >> index 078de0e62..0211069c6 100644 >> --- a/src/box/sql/mem.c >> +++ b/src/box/sql/mem.c >> @@ -613,6 +613,29 @@ mem_set_frame(struct Mem *mem, struct VdbeFrame *frame) >> mem->field_type = field_type_MAX; >> } >> >> +int >> +mem_prepare_aggregate(struct Mem *mem, struct func *func, int size) > > 1. Why is this called 'prepare' when the other setters are called 'set'? > Maybe use 'set' here as well? > Fixed. >> +{ >> + 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; >> + mem->u.func = func; >> + mem->field_type = field_type_MAX; >> + return 0; >> +} >> + >> +void * >> +mem_get_aggregate(struct Mem *mem) >> +{ >> + return (void *)mem->z; > > 2. Void cast should work implicitly here I think. But what is > more interesting is why do you even need the getter? The other > mem types don't have getters for the union fields, and it is > fine I suppose. Would be too much. This one does not even check > if the type is an aggregate. I moved this function from here to patch "sql: introduce mem_get_agg()". I check MEM type there, but almost nothing else changed. This is mostly so because I actually have no idea how to deal with all aggregate functions used standard rules for MEM. Maybe it is worth to think about moving aggregate functions, MEM_Ptr, MEM_Frame, etc from MEM to another internal structure? This way we will be able to use MEMs only for data. New patch: commit 200616f9ee5708f14c565046e2e01eacbef2be62 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 functions stores given functions 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 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; + 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 ffab0d616..cf0db62f9 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -364,6 +364,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 6d9103ff2..c2d4b8b8a 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -403,29 +403,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 @@ -437,12 +414,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; } /*