From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 25 Apr 2019 11:42:34 +0300 From: Alexander Turenko Subject: Re: [tarantool-patches] Re: [PATCH v3 1/1] lua: add key_def lua module Message-ID: <20190425084234.5ipllkz4thd3zxxx@tkn_work_nb> References: <20190424181302.GG13687@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190424181302.GG13687@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org, vdavydov.dev@gmail.com, Kirill Shcherbatov List-ID: > > +static int > > +lbox_key_def_compare_with_key(struct lua_State *L) > > +{ > > + struct key_def *key_def; > > + if (lua_gettop(L) != 3 || > > + (key_def = luaT_check_key_def(L, 1)) == NULL) { > > + return luaL_error(L, "Usage: key_def:" > > + "compare_with_key(tuple, key)"); > > + } > > + > > + struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2); > > + if (tuple == NULL) > > + return luaT_error(L); > > + > > + size_t key_len; > > + const char *key = lbox_encode_tuple_on_gc(L, 3, &key_len); > > + uint32_t part_count = mp_decode_array(&key); > > + if (key_validate_parts(key_def, key, part_count, true) != 0) { > > + tuple_unref(tuple); > > + return luaT_error(L); > > + } > > + > > + int rc = tuple_compare_with_key(tuple, key, part_count, key_def); > > + tuple_unref(tuple); > > + lua_pushinteger(L, rc); > > + return 1; > > +} > > This also looks as a terribly inefficient implementation for > compare. > > Overall, the API looks good to me, while the implementation seems > to be too inefficient. I would consider changing extract_key() to > return char *, not struct tuple, the buffer could be allocated on > transaction region. I also think that compare should not allocate > memory or create tuples, and it should not call tuple_validate() > either. > > If this is urgent, I would push since the code quality is very > good and the api would stable, but I don't see how soo inefficient > compare functions could be useful. We can add *_unchecked() functions, which will not perform validation. I'm against returning char *, because a user can do nothing with it in Lua. We can consider a cursor implementation as motivating example. For a cursor that navigates over an unique index we need :extract_key(), see [1]. Comparing of tuples is needed if we'll try to implement cursors on a non-unique index (but maybe better to implement #3898 ('Allow specifying primary key in iteration by secondary key'). I don't know what Michael F. want when he file #3398 ('lua: introduce key_def module'). Maybe he just needs merger. Maybe we should introduce built-in tuple sorter instead of exposing comparators into Lua, which can be either fast or safe, but not both. [1]: https://github.com/Totktonada/tarantool-merger-examples/blob/695fc9511685033f4b4b22c0df537a12ddf2eaf6/chunked_example_fast/storage.lua#L97 WBR, Alexander Turenko.