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 483876ECE3; Tue, 18 Jan 2022 03:10:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 483876ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1642464608; bh=NJmi7us4dl9bHm0BqTObxod8k8nGlQv5qmWczb8tDW4=; 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=Fw3W7BNIx2tKITQZMq5kIqvKo3HXZc7w9bQ/84LHY9BRgFT6V162kZjPDw/WIsWyt qIw9c+pWU2qQDT1DZgZXs8YhTZZ8Q6enRRYMVHJjrM9YtVNElM4WFQqnrDZz6uL414 KRufeP0GdnoTkVZG6zgFuSayFBiZuQthUTfMYVKM= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 A44026ECE3 for ; Tue, 18 Jan 2022 03:10:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A44026ECE3 Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1n9c4b-0003dz-Ou; Tue, 18 Jan 2022 03:10:06 +0300 Message-ID: <0875aeb4-255d-ce3a-7244-69193cf2f334@tarantool.org> Date: Tue, 18 Jan 2022 01:10:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Content-Language: en-US To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <04154369ec1ff8a1eaf7c9ea1ed37e1fcd1a7120.1642167504.git.imeevma@gmail.com> In-Reply-To: <04154369ec1ff8a1eaf7c9ea1ed37e1fcd1a7120.1642167504.git.imeevma@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9626C4810127D4107B30FD41E156590B5326D62CEB0F0CD52182A05F5380850404C228DA9ACA6FE27CCD0CCA5244EF90BCCA04D620E3BDB719506E2615DA0105A9A905218BFDD3845 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72847AA60176ABEF3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379A6B93796C91DCB58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81C6B025C7C8EF53B407DFCE4FA34F4C2117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BF1175FABE1C0F9B6A471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC046A0DA9F78ADB823AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F790063739B6C7B01977BD37D81D268191BDAD3D698AB9A7B718F8C4D1B931868CE1C5781A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16B5E1C53F199C2BB95B5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: 1B70FBA5C9BEEE72C9761FC34675ADEB871C96603B655635EE9D5CB6078CC77C8A5E5F60F1F9CD74146EF582D857B11F X-C1DE0DAB: 0D63561A33F958A5A6198F5EF72660ED36D00E4BAE66768AB5B7EF38A05118E1D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346B222596F62B8FA9A0FE826EA83C16FF4FFBABABCCF07C552A50D7AC67DBC740CC36D9733F75EDD51D7E09C32AA3244C32FA7E88CBD3C5F80CB126AD94A124178A6D4CC6FBFAC251729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojH1HXIydI9lzOOHwnmmWn7A== X-Mailru-Sender: 1F3202E75A95DDEF4818E0AE2495468D00EB8A5557BFD3F43E98E3200A7FA11AF8D39AE32677A80A07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: introduce user-defined aggregate functions 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! What happened to the idea we had about SQL iterators? Then people would be able to iterate with yields and to make any kind of aggregation without having to create functions for that. See 8 comments below. > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 65c1cb952..f1dfcd807 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -3276,6 +3276,85 @@ func_def_get_ids_from_tuple(struct tuple *tuple, uint32_t *fid, uint32_t *uid) > return tuple_field_u32(tuple, BOX_FUNC_FIELD_UID, uid); > } > > +static int > +func_def_check_body(struct tuple *tuple) > +{ > + assert(tuple_field_count(tuple) > BOX_FUNC_FIELD_BODY); > + const char *field = tuple_field(tuple, BOX_FUNC_FIELD_BODY); > + assert(field != NULL); > + enum mp_type type = mp_typeof(*field); > + if (type == MP_STR) > + return 0; > + if (type != MP_MAP) { > + diag_set(ClientError, ER_FIELD_TYPE, "map or array", 1. 'Map or string'. > + mp_type_strs[type]); > + return -1; > + } > + const char *agg = tuple_field_cstr(tuple, BOX_FUNC_FIELD_AGGREGATE); > + if (agg == NULL) > + return -1; > + if (STR2ENUM(func_aggregate, agg) != FUNC_AGGREGATE_GROUP) { > + const char *name = tuple_field_cstr(tuple, BOX_FUNC_FIELD_NAME); > + diag_set(ClientError, ER_CREATE_FUNCTION, name, > + "only aggregate functions can have map as body"); > + return -1; > + } > + > + bool has_step = false; > + bool has_finalize = false; > + if (mp_decode_map(&field) != 2) > + goto error; > + for (int i = 0; i < 2; ++i) { > + const char *value; > + uint32_t size; > + if (mp_typeof(*field) != MP_STR) > + goto error; > + value = mp_decode_str(&field, &size); > + if (size == strlen("step")) { > + if (strncmp(value, "step", size) != 0) > + goto error; 2. I would rather propose to introduce a function to compare 0-terminated string with non-terminated one. It is needed quite often apparently. For instance, strlcmp(const char *l, const char *r, size_t r_len); > + has_step = true; > + } else if (size == strlen("finalize")) { > + if (strncmp(value, "finalize", size) != 0) > + goto error; > + has_finalize = true; > + } > + if (mp_typeof(*field) != MP_STR) > + goto error; > + mp_next(&field); > + } > + if (has_step && has_finalize) > + return 0; > +error: > + const char *name = tuple_field_cstr(tuple, BOX_FUNC_FIELD_NAME); > + diag_set(ClientError, ER_CREATE_FUNCTION, name, > + "body of aggregate function should be map that contains " > + "exactly two string fields: 'step' and 'finalize'"); > + return -1; > +} > + > +static const char * > +func_def_get_agg_body(struct tuple *tuple, uint32_t *body_len, bool is_step) 3. Please, lets split that function in 2 maybe. Its invocation with hardcoded 'true'/'false' in the end gives hard time understanding what these values mean. > +{ > + const char *field = tuple_field(tuple, BOX_FUNC_FIELD_BODY); > + assert(field != NULL); > + if (mp_typeof(*field) == MP_STR) { > + if (is_step) > + return mp_decode_str(&field, body_len); > + *body_len = 0; > + return field; 4. You return len as 0, but the returned value != NULL? Why? > + } > @@ -3568,6 +3697,16 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) > struct func *func = func_new(def); > if (func == NULL) > return -1; > + if (def->aggregate == FUNC_AGGREGATE_GROUP) { > + struct func_def *fin_def = > + func_fin_def_new(new_tuple, def); > + if (fin_def == NULL) > + return -1; > + struct func *func_fin = func_new(fin_def); > + if (func_fin == NULL) > + return -1; > + func_fin_cache_insert(func_fin); 5. What happens if WAL write fails and on_create_func_rollback() is called? > diff --git a/src/box/schema.h b/src/box/schema.h > index d3bbdd590..2f020ee96 100644 > --- a/src/box/schema.h > +++ b/src/box/schema.h > @@ -102,6 +102,15 @@ func_by_id(uint32_t fid); > struct func * > func_by_name(const char *name, uint32_t name_len); > > +void > +func_fin_cache_insert(struct func *func); > + > +void > +func_fin_cache_delete(uint32_t fid); > + > +struct func * > +func_fin_by_id(uint32_t fid); 6. Lets leave some comments about what is 'func fin'. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 2e33a31d2..c77cd1c9f 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -5644,10 +5650,17 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo) > pParse->is_aborted = true; > return; > } > - sqlVdbeAddOp3(v, OP_AggStep, nArg, regAgg, pF->iMem); > - sqlVdbeAppendP4(v, ctx, P4_FUNCCTX); > - sql_expr_type_cache_change(pParse, regAgg, nArg); > - sqlReleaseTempRange(pParse, regAgg, nArg); > + if (pF->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) { > + sqlVdbeAddOp3(v, OP_AggStep, nArg, regAgg, pF->iMem); > + sqlVdbeAppendP4(v, ctx, P4_FUNCCTX); > + } else { > + sqlVdbeAddOp3(v, OP_UserAggStep, nArg, regAgg, > + pF->iMem); > + const char *name = pF->func->def->name; > + int len = pF->func->def->name_len; > + char *str = sqlDbStrNDup(pParse->db, name, len); > + sqlVdbeAppendP4(v, str, P4_DYNAMIC); 7. Was it possible to unify how builtin and user-defined functions are called? In the same opcode without any special 'if's. > diff --git a/test/sql-tap/gh-2579-custom-aggregate.test.lua b/test/sql-tap/gh-2579-custom-aggregate.test.lua > new file mode 100755 > index 000000000..d5b845761 > --- /dev/null > +++ b/test/sql-tap/gh-2579-custom-aggregate.test.lua 8. It might also be good to have a test with step/finalize functions throwing a Lua error from their body.