From: Kirill Shcherbatov <kshcherbatov@tarantool.org> To: tarantool-patches@freelists.org, korablev@tarantool.org Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org> Subject: [tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem Date: Wed, 21 Aug 2019 18:28:05 +0300 [thread overview] Message-ID: <cover.1566400979.git.kshcherbatov@tarantool.org> (raw) 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
next reply other threads:[~2019-08-21 15:28 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-21 15:28 Kirill Shcherbatov [this message] 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 1/4] sql: rename sql_vdbe_mem_alloc_region helper Kirill Shcherbatov 2019-08-22 13:04 ` [tarantool-patches] " n.pettik 2019-08-23 15:02 ` Kirill Shcherbatov 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 2/4] sql: replace flag MINMAX with flags MIN and MAX Kirill Shcherbatov 2019-08-22 13:30 ` [tarantool-patches] " n.pettik 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 3/4] sql: get rid of FuncDef function hash Kirill Shcherbatov 2019-08-22 14:37 ` [tarantool-patches] " n.pettik 2019-08-23 15:02 ` [tarantool-patches] [PATCH v4 4/5] " Kirill Shcherbatov 2019-08-23 15:02 ` [tarantool-patches] [PATCH v4 3/5] sql: remove name overloading for SQL builtins Kirill Shcherbatov 2019-08-28 15:05 ` [tarantool-patches] " Nikita Pettik 2019-08-23 15:02 ` [tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash Kirill Shcherbatov 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 4/4] sql: support user-defined functions in SQL Kirill Shcherbatov 2019-08-22 15:23 ` [tarantool-patches] " n.pettik 2019-08-23 15:02 ` Kirill Shcherbatov 2019-08-29 15:09 ` [tarantool-patches] Re: [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem Nikita Pettik 2019-08-29 17:12 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cover.1566400979.git.kshcherbatov@tarantool.org \ --to=kshcherbatov@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='Re: [tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox