Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem
@ 2019-08-21 15:28 Kirill Shcherbatov
  2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 1/4] sql: rename sql_vdbe_mem_alloc_region helper Kirill Shcherbatov
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Kirill Shcherbatov @ 2019-08-21 15:28 UTC (permalink / raw)
  To: tarantool-patches, korablev; +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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-08-29 17:12 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-21 15:28 [tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem Kirill Shcherbatov
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox