From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 43C0E25F7D for ; Tue, 13 Aug 2019 16:43:14 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4EkIKv6m8I9h for ; Tue, 13 Aug 2019 16:43:14 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C8A2F251E9 for ; Tue, 13 Aug 2019 16:43:13 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH v2 8/8] box: get rid of box.internal.sql_function_create From: "n.pettik" In-Reply-To: <8c42515c61f85e03dcd5abe35055b9301379de0a.1565275470.git.kshcherbatov@tarantool.org> Date: Tue, 13 Aug 2019 23:43:11 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <8c42515c61f85e03dcd5abe35055b9301379de0a.1565275470.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov > 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) <=3D = sizeof(struct port), > void > port_lua_create(struct port *port, struct lua_State *L); >=20 > +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=E2=80=99d 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); > }; >=20 >=20 > 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 > #include > #include "box/coll_id_cache.h" > +#include "box/port.h" > +#include "box/tuple.h" > +#include "lua/utils.h" > +#include "mpstream.h" >=20 > /* > * Return the collating function associated with a function. > @@ -70,6 +74,279 @@ sqlSkipAccumulatorLoad(sql_context * context) > context->skipFlag =3D 1; > } >=20 > +/** > + * 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 =3D &fiber()->gc; > + struct Mem *ret =3D region_alloc(region, count * sizeof(*ret)); > + if (ret =3D=3D NULL) { > + diag_set(OutOfMemory, count * sizeof(*ret), > + "region_alloc", "ret"); > + return NULL; > + } > + memset(ret, 0, count * sizeof(*ret)); > + for (uint32_t i =3D 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 =3D (struct port_vdbemem *) base; > + struct region *region =3D &fiber()->gc; > + size_t region_svp =3D region_used(region); > + bool is_error =3D 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 =3D 0; i < port->size && !is_error; i++) { > + sql_value *param =3D > + (sql_value *)((struct Mem *)port->mem + i); > + switch (sql_value_type(param)) { > + case MP_INT: { > + sql_int64 val =3D sql_value_int64(param); > + if (val < 0) { > + mpstream_encode_int(&stream, val); > + break; > + } > + FALLTHROUGH; > + } > + case MP_UINT: { > + sql_int64 val =3D 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 >=20 > @@ -1767,6 +1768,50 @@ case OP_BuiltinFunction: { > break; > } >=20 > +/* Opcode: Function P1 P2 P3 P4 P5 > + * Synopsis: r[P3]=3Dfunc(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=E2=80=99t 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 =3D pOp->p4.func; > + int argc =3D pOp->p5; > + struct Mem *argv =3D &aMem[pOp->p2]; > + struct port args, ret; > + > + struct region *region =3D &fiber()->gc; > + size_t region_svp =3D region_used(region); > + port_vdbemem_create(&args, (struct sql_value *)argv, argc); > + if (func_call(func, &args, &ret) !=3D 0) > + goto abort_due_to_error; > + > + pOut =3D vdbe_prepare_null_out(p, pOp->p3); > + uint32_t size; > + struct Mem *mem =3D (struct Mem *)port_get_vdbemem(&ret, &size); > + if (mem !=3D NULL && size > 0) > + *pOut =3D mem[0]; > + port_destroy(&ret); > + region_truncate(region, region_svp); > + if (mem =3D=3D 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 =3D=3D NULL) goto abort_due_to_error; =20 - /* 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)) !=3D 0) if (sqlVdbeMemTooBig(pOut)) goto too_big; =20 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]=3Dr[P1]&r[P2] > * >=20 > 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: =E2=80=9Cinvalid number of arguments is passed to %s: expected %d, got = %d"