[tarantool-patches] Re: [PATCH v2 8/8] box: get rid of box.internal.sql_function_create
n.pettik
korablev at tarantool.org
Tue Aug 13 23:43:11 MSK 2019
> diff --git a/src/box/func_def.h b/src/box/func_def.h
> index d99d89190..988d080f0 100644
> --- a/src/box/func_def.h
> +++ b/src/box/func_def.h
> @@ -110,7 +110,7 @@ struct func_def {
> */
> bool is_sandboxed;
> /** The count of function's input arguments. */
> - int param_count;
> + uint32_t param_count;
> /** The type of the value returned by function. */
> enum field_type returns;
> /** Function aggregate option. */
> diff --git a/src/box/port.h b/src/box/port.h
> index a7f5d81bd..26f85b145 100644
> --- a/src/box/port.h
> +++ b/src/box/port.h
> @@ -113,6 +113,23 @@ static_assert(sizeof(struct port_lua) <= sizeof(struct port),
> void
> port_lua_create(struct port *port, struct lua_State *L);
>
> +struct sql_value;
> +struct sql_context;
Nit: forward declaration of sql_context is redundant.
> +
> +/** Port implementation used with vdbe memory variables. */
> +struct port_vdbemem {
> + const struct port_vtab *vtab;
> + struct sql_value *mem;
> + uint32_t size;
Nit: I’d better call it mem_count (or at least add brief explanation comment).
> diff --git a/src/lib/core/port.h b/src/lib/core/port.h
> index 09a026df5..17096d0d0 100644
> --- a/src/lib/core/port.h
> +++ b/src/lib/core/port.h
> @@ -98,6 +98,19 @@ struct port_vtab {
> * is responsible for cleaning up.
> **/
> const char *(*get_msgpack)(struct port *port, uint32_t *size);
> + /**
> + * Get the content of a port as a sequence of vdbe memory
> + * variables. The SQL VDBE uses this representation for
> + * process data. This API is usefull to pass VDBE
> + * variables to sql builtin functions.
> + * The lifecycle of the returned value is
> + * implementation-specific: it may either be returned
> + * directly from the port, in which case the data will
> + * stay alive as long as the port is alive, or it may be
> + * allocated on the fiber()->gc, in which case the caller
> + * is responsible for cleaning up.
> + */
Consider comment fixes:
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index 17096d0d0..c6ca115a1 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -99,16 +99,12 @@ struct port_vtab {
**/
const char *(*get_msgpack)(struct port *port, uint32_t *size);
/**
- * Get the content of a port as a sequence of vdbe memory
- * variables. The SQL VDBE uses this representation for
- * process data. This API is usefull to pass VDBE
- * variables to sql builtin functions.
- * The lifecycle of the returned value is
- * implementation-specific: it may either be returned
- * directly from the port, in which case the data will
- * stay alive as long as the port is alive, or it may be
- * allocated on the fiber()->gc, in which case the caller
- * is responsible for cleaning up.
+ * Get the content of a port as a sequence of VDBE memory
+ * cells. This API is used to pass VDBE variables to
+ * user-defined Lua functions (see OP_Function in sql/vdbe.c).
+ * The lifecycle of the returned value is the same as for
+ * @get_msgpack method, i.e. it depends on particular
+ * implementation.
*/
struct sql_value *(*get_vdbemem)(struct port *port, uint32_t *size);
/** Destroy a port and release associated resources. */
> + struct sql_value *(*get_vdbemem)(struct port *port, uint32_t *size);
> /** Destroy a port and release associated resources. */
> void (*destroy)(struct port *port);
> };
>
>
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index f07c52b95..ea94d6151 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -45,6 +45,10 @@
> #include <unicode/uchar.h>
> #include <unicode/ucol.h>
> #include "box/coll_id_cache.h"
> +#include "box/port.h"
> +#include "box/tuple.h"
> +#include "lua/utils.h"
> +#include "mpstream.h"
>
> /*
> * Return the collating function associated with a function.
> @@ -70,6 +74,279 @@ sqlSkipAccumulatorLoad(sql_context * context)
> context->skipFlag = 1;
> }
>
> +/**
> + * Allocate a sequence of initialized vdbe memory registers
> + * on region.
> + */
> +static struct Mem *
> +vdbemem_alloc_on_region(uint32_t count)
Nit: we already have sql_vdbe_mem_alloc_region()
which allocates memory for string using region.
Could you rename it (the original one I mean) to avoid
any confusions?
> +{
> + struct region *region = &fiber()->gc;
> + struct Mem *ret = region_alloc(region, count * sizeof(*ret));
> + if (ret == NULL) {
> + diag_set(OutOfMemory, count * sizeof(*ret),
> + "region_alloc", "ret");
> + return NULL;
> + }
> + memset(ret, 0, count * sizeof(*ret));
> + for (uint32_t i = 0; i < count; i++) {
> + sqlVdbeMemInit(&ret[i], sql_get(), MEM_Null);
> + assert(memIsValid(&ret[i]));
> + }
> + return ret;
> +}
> +
> +static const char *
> +port_vdbemem_get_msgpack(struct port *base, uint32_t *size)
In fact, this function is unused and is not tested at all.
> +{
> + struct port_vdbemem *port = (struct port_vdbemem *) base;
> + struct region *region = &fiber()->gc;
> + size_t region_svp = region_used(region);
> + bool is_error = false;
> + struct mpstream stream;
> + mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> + set_encode_error, &is_error);
> + mpstream_encode_array(&stream, port->size);
> + for (uint32_t i = 0; i < port->size && !is_error; i++) {
> + sql_value *param =
> + (sql_value *)((struct Mem *)port->mem + i);
> + switch (sql_value_type(param)) {
> + case MP_INT: {
> + sql_int64 val = sql_value_int64(param);
> + if (val < 0) {
> + mpstream_encode_int(&stream, val);
> + break;
> + }
> + FALLTHROUGH;
> + }
> + case MP_UINT: {
> + sql_int64 val = sql_value_int64(param);
> + mpstream_encode_uint(&stream, val);
> + break;
> + }
> + case MP_DOUBLE: {
> + mpstream_encode_double(&stream,
> + sql_value_double(param));
> + break;
> + }
> + case MP_STR: {
> + mpstream_encode_str(&stream,
> + (const char *) sql_value_text(param));
Nit: a bit broken indentation. You can use auxiliary tmp var.
> + break;
> + }
> + case MP_BIN: {
> + mpstream_encode_binl(&stream, sql_value_bytes(param));
> + mpstream_memcpy(&stream, sql_value_blob(param),
> + sql_value_bytes(param));
> + break;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a66becc89..23b6995d7 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
>
> @@ -1767,6 +1768,50 @@ case OP_BuiltinFunction: {
> break;
> }
>
> +/* Opcode: Function P1 P2 P3 P4 P5
> + * Synopsis: r[P3]=func(r[P2 at P5])
> + *
> + * Invoke a user function (P4 is a pointer to a function object
> + * that defines the function) with P5 arguments taken from
> + * register P2 and successors. The result of the function is
> + * stored in register P3.
> + * Register P3 must not be one of the function inputs.
Nit: you don’t check this fact.
> + *
> + * P1 is a 32-bit bitmask indicating whether or not each argument
> + * to the function was determined to be constant at compile time.
> + * If the first argument was constant then bit 0 of P1 is set.
This comment is not related to the code: P1 is not involved at all.
> + */
> +case OP_Function: {
> + struct func *func = pOp->p4.func;
> + int argc = pOp->p5;
> + struct Mem *argv = &aMem[pOp->p2];
> + struct port args, ret;
> +
> + struct region *region = &fiber()->gc;
> + size_t region_svp = region_used(region);
> + port_vdbemem_create(&args, (struct sql_value *)argv, argc);
> + if (func_call(func, &args, &ret) != 0)
> + goto abort_due_to_error;
> +
> + pOut = vdbe_prepare_null_out(p, pOp->p3);
> + uint32_t size;
> + struct Mem *mem = (struct Mem *)port_get_vdbemem(&ret, &size);
> + if (mem != NULL && size > 0)
> + *pOut = mem[0];
> + port_destroy(&ret);
> + region_truncate(region, region_svp);
> + if (mem == NULL)
> + goto abort_due_to_error;
> +
> + /* Copy the result of the function into register P3 */
> + if (pOut->flags & (MEM_Str|MEM_Blob))
Nit:
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 23b6995d7..69cbab45c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1803,8 +1803,11 @@ case OP_Function: {
if (mem == NULL)
goto abort_due_to_error;
- /* Copy the result of the function into register P3 */
- if (pOut->flags & (MEM_Str|MEM_Blob))
+ /*
+ * Copy the result of the function invocation into
+ * register P3.
+ */
+ if ((pOut->flags & (MEM_Str | MEM_Blob)) != 0)
if (sqlVdbeMemTooBig(pOut)) goto too_big;
REGISTER_TRACE(p, pOp->p3, pOut);
> + if (sqlVdbeMemTooBig(pOut)) goto too_big;
> +
> + REGISTER_TRACE(p, pOp->p3, pOut);
> + UPDATE_MAX_BLOBSIZE(pOut);
> + break;
> +}
> +
> /* Opcode: BitAnd P1 P2 P3 * *
> * Synopsis: r[P3]=r[P1]&r[P2]
> *
>
> diff --git a/test/box/function1.result b/test/box/function1.result
> index 5b091f72b..48c9ad6cf 100644
> --- a/test/box/function1.result
> +++ b/test/box/function1.result
> @@ -366,6 +366,177 @@ test_run:cmd("setopt delimiter ''");
> +box.execute('SELECT "function1.divide"()')
> +---
> +- null
> +- invalid argument
> +...
> +box.execute('SELECT "function1.divide"(6)')
> +---
> +- null
> +- wrong number of arguments to function function1.divide()
> +...
> +box.execute('SELECT "function1.divide"(6, 3)')
> +---
> +- null
> +- wrong number of arguments to function function1.divide()
Oh, could you please fix this awful error message?
At least, it lacks a verb. Personally I would prefer smth like:
“invalid number of arguments is passed to %s: expected %d, got %d"
More information about the Tarantool-patches
mailing list