From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A1E5A275F6 for ; Wed, 21 Aug 2019 11:28:13 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 49HoLi9MBs8l for ; Wed, 21 Aug 2019 11:28:13 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D597B274CC for ; Wed, 21 Aug 2019 11:28:12 -0400 (EDT) From: Kirill Shcherbatov Subject: [tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem Date: Wed, 21 Aug 2019 18:28:05 +0300 Message-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, korablev@tarantool.org Cc: Kirill Shcherbatov Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4182-persistent-functions Issue: https://github.com/tarantool/tarantool/issues/2233 Kirill Shcherbatov (4): sql: rename sql_vdbe_mem_alloc_region helper sql: replace flag MINMAX with flags MIN and MAX sql: get rid of FuncDef function hash sql: support user-defined functions in SQL Changes in v4: - removed check_param_count method; use dynamic checks instead - user can't create a builtin anymore - changes are split for 3 patches - many minor fixes I'll cite @Korablev's review comments below: > ‘user_data' is not used anymore, please remove. done > + */ > + int (*check_param_count)(struct func_sql_builtin *func, int argc); > Check whether function supports given number of arguments. > For instance, trim() function can accept one, two or three arguments. Applied. > Again: pMem -> mem. done > To be honest, all these stubs and idea with count_check > function at all now seems to be not as good as it used to be. > I’ve removed this check and instead added several runtime > ones and IMHO it looks way much better: I've adopted a code on your branch. > I’d rather move introduction of SQL_FUNC_MAX flag to a separate patch. done > Nit: one extra empty line. fixed > I’d rename it to func_sql_builtin_call_stub() done > Let’s reorganise a bit this struct: done > Now you can use memcpy() to copy corresponding parts > to struct func_def and struct func_sql_builtin (it is up to you) I don't like memcpy in this case > Please, leave one empty line between structs, so that > one can visually separate them: done > func.h and func_def.h seem to be redundant - > I’ve removed them and project has compiled. done > This makes you include box/schema.h discussed in Telegram; let's keep it. > "No such function” and “not available in SQL” are different errors. > Please, handle both cases with proper error messages and add > test cases. Implemented. > == FUNC_AGGREGATE — check on language is redundant. > Instead, add an assertion. assertion is ok > Nit: sql_func_flag_is_set() returns boolean, no need to check > it on equality to zero. fixed > Nit: …SQL built-in. Fixed > > Renamed to sql_vdbe_mem_alloc_blob_region > Please, move this fix to a separate patch. Done. > As a rule, users don’t even open source code. > This is solely developer oriented comment, so either place > assertion verifying that P3 doesn’t get into mentioned range > (if it is really vital) or remove this comment. I've drop this comment. > Why should it be? > Also you skipped one nit: > - if (pOut->flags & (MEM_Str|MEM_Blob)) > -> > + if ((pOut->flags & (MEM_Str | MEM_Blob)) != 0) > if (sqlVdbeMemTooBig(pOut)) goto too_big; > Please, apply. Done. > Builting -> built-in; doesn’t -> don't Done. > > * method to gain the best possible performance for SQL > > * requests. > Can’t parse this sentence at all. Updated the whole comment: /** * A VDBE-memory-compatible call method. * SQL built-ins don't use func base class "call" * method to provide a best performance for SQL requests. * Access checks are redundant, because all SQL built-ins * are predefined and are executed on SQL privilege level. */ void (*call)(sql_context *ctx, int argc, sql_value **argv); > Well, now we have another problem: user can create > built-in function, but can’t drop it: > tarantool> box.schema.func.create('WAITFOR', > {language = 'SQL_BUILTIN', param_list = {'integer'}, > returns = 'integer',exports = {'SQL'}}) Solved. Included in test coverage. > >> user_data is used only for min/max functions. Let’s consider removing it. > > Removed user_data at all. > I’d better move it in a separate commit. Verbally discussed, why it is not ok. src/box/errcode.h | 1 + src/box/lua/lua_sql.h | 39 -- src/box/port.h | 17 + src/box/sql.h | 1 + src/box/sql/sqlInt.h | 234 ++----- src/box/sql/vdbe.h | 11 +- src/box/sql/vdbeInt.h | 23 +- src/lib/core/port.h | 15 + src/box/call.c | 1 + src/box/execute.c | 1 + src/box/func.c | 33 +- src/box/lua/call.c | 6 +- src/box/lua/lua_sql.c | 205 ------ src/box/port.c | 4 + src/box/sql/analyze.c | 34 +- src/box/sql/callback.c | 204 ------ src/box/sql/date.c | 28 - src/box/sql/expr.c | 83 ++- src/box/sql/func.c | 1051 +++++++++++++++++++++++++++---- src/box/sql/global.c | 7 - src/box/sql/main.c | 137 ---- src/box/sql/resolve.c | 79 ++- src/box/sql/select.c | 10 +- src/box/sql/vdbe.c | 65 +- src/box/sql/vdbeapi.c | 17 +- src/box/sql/vdbeaux.c | 33 +- src/box/sql/vdbemem.c | 65 +- src/box/sql/whereexpr.c | 2 +- src/box/CMakeLists.txt | 1 - src/box/alter.cc | 6 + src/box/lua/schema.lua | 3 +- test/box/function1.result | 196 ++++++ test/box/function1.test.lua | 70 ++ test/box/misc.result | 1 + test/sql-tap/alias.test.lua | 11 +- test/sql-tap/check.test.lua | 11 +- test/sql-tap/func.test.lua | 22 +- test/sql-tap/func2.test.lua | 18 +- test/sql-tap/func5.test.lua | 27 +- test/sql-tap/limit.test.lua | 4 +- test/sql-tap/lua_sql.test.lua | 121 ++-- test/sql-tap/select1.test.lua | 12 +- test/sql-tap/subquery.test.lua | 21 +- test/sql-tap/trigger9.test.lua | 8 +- test/sql-tap/where2.test.lua | 4 +- test/sql/errinj.result | 26 - test/sql/errinj.test.lua | 10 - test/sql/func-recreate.result | 41 +- test/sql/func-recreate.test.lua | 26 +- test/sql/icu-upper-lower.result | 4 +- 50 files changed, 1751 insertions(+), 1298 deletions(-) delete mode 100644 src/box/lua/lua_sql.h delete mode 100644 src/box/lua/lua_sql.c -- 2.22.1