[tarantool-patches] Re: [PATCH v2 8/8] box: get rid of box.internal.sql_function_create

["n.pettik" <korablev@tarantool.org>]

> 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@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"