[tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem
Kirill Shcherbatov
kshcherbatov at tarantool.org
Wed Aug 21 18:28:05 MSK 2019
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
More information about the Tarantool-patches
mailing list