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 6A98C273A5 for ; Mon, 2 Apr 2018 06:26:50 -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 0u6Au5cL_30x for ; Mon, 2 Apr 2018 06:26:50 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 7D07827399 for ; Mon, 2 Apr 2018 06:26:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple References: <90ece9a705197ded006ef50155f3b1bfca4c8d8f.1522261662.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: <7a5c0afb-c030-fcd2-7d86-5c92cd7fa850@tarantool.org> Date: Mon, 2 Apr 2018 13:26:46 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: "v.shpilevoy@tarantool.org" Comments accounted. On 29.03.2018 13:14, v.shpilevoy@tarantool.org wrote: > See below 10 comments. >=20 >> diff --git a/src/box/lua/space.cc=20 >> =A0b/src/box/lua/space.cc >> index 29a9aca..a8cfb14 100644 >> --- a/src/box/lua/space.cc >> +++ b/src/box/lua/space.cc >> @@ -32,6 +32,7 @@ >> #include "box/lua/tuple.h" >> #include "lua/utils.h" >> #include "lua/trigger.h" >> +#include "box/schema.h" >> >> extern "C" { >> #include >> @@ -174,6 +175,16 @@ lbox_fillspace(struct lua_State *L, struct space=20 >> *space, int i) >> lua_pushstring(L, space->def->engine_name); >> lua_settable(L, i); >> >> +/* space.schema_version */ >> +lua_pushstring(L, "schema_version"); >> +luaL_pushuint64(L, box_schema_version()); >> +lua_settable(L, i); >=20 > 1. Lets name it '_schema_version' - it is internal field. >=20 >> + >> +/* space._space */ >> +lua_pushstring(L, "_space"); >> +lua_pushlightuserdata(L, space); >> +lua_settable(L, i); >=20 > 2. Please, find a better name for the pointer. For example, 'ptr' - then = it > will be accessed as space.ptr, that looks slightly better than space._spa= ce. >=20 >> +static int >> +lbox_space_ptr_cached(struct lua_State *L) >> +{ >=20 diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua index 740b885..c2645f7 100644 --- a/src/box/lua/schema.lua +++ b/src/box/lua/schema.lua @@ -1420,7 +1420,7 @@ function box.schema.space.bless(space) builtin.space_run_triggers(s, yesno) end space_mt.frommap =3D box.internal.space.frommap - space_mt._ptr =3D box.internal.space._ptr + space_mt._cptr =3D box.internal.space._cptr space_mt.__index =3D space_mt setmetatable(space, space_mt) diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc index 42eaf79..f8583a2 100644 --- a/src/box/lua/space.cc +++ b/src/box/lua/space.cc @@ -176,12 +176,12 @@ lbox_fillspace(struct lua_State *L, struct space=20 *space, int i) lua_settable(L, i); /* space.schema_version */ - lua_pushstring(L, "schema_version"); + lua_pushstring(L, "_schema_version"); luaL_pushuint64(L, box_schema_version()); lua_settable(L, i); - /* space._space */ - lua_pushstring(L, "_space"); + /* space._cptr */ + lua_pushstring(L, "_cptr"); lua_pushlightuserdata(L, space); lua_settable(L, i); > 3. Add a comment, what this function does, why, and what arguments on a=20 > stack > it expects, what and home many values returns. >=20 >> +// take care about stack to call this function in frommap directly >> +lua_getfield(L, 1, "schema_version"); >=20 > 4. A comment seems to be not linked with the code below. And please, do n= ot > use '//' comments. In our code we use '/* ... */' for comments inside a > function. Note, that comment max length is 66 symbols, including indentat= ion > before a comment. >=20 >> + >> +void *space =3D nullptr; >> +if (schema_version =3D=3D global_shema_version) { >> + >=20 > 5. Do not use nullptr. Everywhere in Tarantool code only NULL is used. >=20 >> +bool tuple =3D false; >=20 > 6. Lets return tuple by default, and name the option 'table' instead of > tuple. If a user wants a table, then he must pass the option {table =3D t= rue}. >=20 >> +if (argc =3D=3D 3) { >> +if (!lua_istable(L, 3)) >> +goto error; >> +lua_getfield(L, 3, "tuple"); >> +if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) >> +goto error; >> +tuple =3D lua_toboolean(L, -1); >> +} >> + >=20 > 7. Double tabs prior to 'goto error'. >=20 >> +if (tuple_fieldno_by_name(dict, key, key_len, key_hash, &fieldno)) >> +luaL_error(L, "Unknown field '%s'", key); >=20 > 8. According to our Lua error returning convention, you can raise an=20 > error either > on incorrect usage (bad argument type, for example), or on OOM(Out Of=20 > Memory) > errors. On other errors you must return two values: nil and error object = or > description. In this example you must return nil and "Unknown field=20 > '%s'" string. >=20 >> +lua_replace(L, 1); >> +lua_settop(L, 1); >> +return lbox_tuple_new(L); @@ -397,56 +397,66 @@ static struct trigger on_alter_space_in_lua =3D { RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL }; +/** + * Get a C pointer for space structure. + * Caches the result. Cleans the user(negative) stack itself. + * @param Lua space object at top of the Lua stack + * @return C structure pointer + */ static int lbox_space_ptr_cached(struct lua_State *L) { if (lua_gettop(L) < 1 || !lua_istable(L, 1)) - luaL_error(L, "Usage: space:_ptr_cached()"); + luaL_error(L, "Usage: space:_cptr()"); - // take care about stack to call this function in frommap directly - lua_getfield(L, 1, "schema_version"); + lua_getfield(L, 1, "_schema_version"); uint64_t schema_version =3D luaL_touint64(L, -1); lua_pop(L, 1); uint64_t global_shema_version =3D box_schema_version(); - void *space =3D nullptr; + void *space =3D NULL; if (schema_version =3D=3D global_shema_version) { - lua_getfield(L, 1, "_space"); + lua_getfield(L, 1, "_cptr"); space =3D (void *)lua_topointer(L, -1); - lua_pop(L, 1); } else { lua_getfield(L, 1, "id"); uint32_t id =3D luaL_touint64(L, -1); - lua_pop(L, 1); space =3D space_by_id(id); + /** update the cache */ luaL_pushuint64(L, global_shema_version); - lua_setfield(L, 1, "schema_version"); - + lua_setfield(L, 1, "_schema_version"); lua_pushlightuserdata(L, space); - lua_setfield(L, 1, "_space"); + lua_setfield(L, 1, "_cptr"); } - + lua_pop(L, 1); lua_pushlightuserdata(L, space); return 1; } +/** + * Make a tuple or a table Lua object by map + * @param Lua space object + * @param Lua map table object + * @param Lua opts table object (optional) + * @return C structure pointer + */ static int lbox_space_frommap(struct lua_State *L) { - struct tuple_dictionary *dict =3D nullptr; - struct space *space =3D nullptr; + struct tuple_dictionary *dict =3D NULL; + struct space *space =3D NULL; int argc =3D lua_gettop(L); - bool tuple =3D false; + bool table =3D false; if (argc < 2 || argc > 3 || !lua_istable(L, 2)) goto error; if (argc =3D=3D 3) { if (!lua_istable(L, 3)) - goto error; - lua_getfield(L, 3, "tuple"); + goto error; + lua_getfield(L, 3, "table"); if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) - goto error; - tuple =3D lua_toboolean(L, -1); + goto error; + table =3D lua_toboolean(L, -1); } lbox_space_ptr_cached(L); @@ -461,11 +471,14 @@ lbox_space_frommap(struct lua_State *L) size_t key_len; const char *key =3D lua_tolstring(L, -2, &key_len); uint32_t key_hash =3D lua_hashstring(L, -2); - if (tuple_fieldno_by_name(dict, key, key_len, key_hash, &fieldno)) - luaL_error(L, "Unknown field '%s'", key); + if (tuple_fieldno_by_name(dict, key, key_len, key_hash, &fieldno)) { + lua_pushnil(L); + lua_pushstring(L, "Unknown field"); + return 2; + } lua_rawseti(L, -3, fieldno+1); } - if (!tuple) + if (table) return 1; lua_replace(L, 1); @@ -557,7 +570,7 @@ box_lua_space_init(struct lua_State *L) static const struct luaL_Reg space_internal_lib[] =3D { {"frommap", lbox_space_frommap}, - {"_ptr", lbox_space_ptr_cached}, + {"_cptr", lbox_space_ptr_cached}, {NULL, NULL} }; luaL_register(L, "box.internal.space", space_internal_lib); >=20 > 9. How about a comment what is happening here? >=20 > 10. Add a test on nil and box.NULL fields. And test, that a result of > frommap() actually can be inserted or replaced into a space. >=20 >=20 diff --git a/test/box/space_frommap.result b/test/box/space_frommap.result index 884d78c..7a4b3db 100644 --- a/test/box/space_frommap.result +++ b/test/box/space_frommap.result @@ -29,21 +29,16 @@ subscriber =3D box.schema.space.create('subscriber',=20 {format =3D format}) ... subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}) --- -- - 2 - - 4 - - 3 - - 1 +- [2, 4, 3, 1] ... subscriber:frommap({ddd =3D 1, aaa =3D 2, bbb =3D 3}) --- -- - 2 - - 3 - - null - - 1 +- [2, 3, null, 1] ... subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, eee =3D 4}) --- -- error: Unknown field 'eee' +- null +- Unknown field ... subscriber:frommap() --- @@ -53,23 +48,34 @@ subscriber:frommap({}) --- - [] ... -subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {tuple = =3D true}) ---- -- [2, 4, 3, 1] -... -subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {tuple = =3D false}) +subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {table = =3D true}) --- - - 2 - 4 - 3 - 1 ... +subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {table = =3D false}) +--- +- [2, 4, 3, 1] +... +subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D box.NULL}) +--- +- [2, null, 3, 1] +... subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {dummy = =3D true}) --- -- - 2 - - 4 - - 3 - - 1 +- [2, 4, 3, 1] +... +_ =3D subscriber:create_index('primary', {parts=3D{'aaa'}}) +--- +... +tuple =3D subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}) +--- +... +subscriber:replace(tuple) +--- +- [2, 4, 3, 1] ... subscriber:drop() --- diff --git a/test/box/space_frommap.test.lua=20 b/test/box/space_frommap.test.lua index 4760b3a..ff7ef98 100644 --- a/test/box/space_frommap.test.lua +++ b/test/box/space_frommap.test.lua @@ -14,7 +14,11 @@ subscriber:frommap({ddd =3D 1, aaa =3D 2, bbb =3D 3}) subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, eee =3D 4}) subscriber:frommap() subscriber:frommap({}) -subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {tuple = =3D true}) -subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {tuple = =3D false}) +subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {table = =3D true}) +subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {table = =3D false}) +subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D box.NULL}) subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}, {dummy = =3D true}) +_ =3D subscriber:create_index('primary', {parts=3D{'aaa'}}) +tuple =3D subscriber:frommap({ddd =3D 1, aaa =3D 2, ccc =3D 3, bbb =3D 4}) +subscriber:replace(tuple) subscriber:drop()