Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object
Date: Wed, 27 Mar 2019 01:54:08 +0300	[thread overview]
Message-ID: <20190326225408.tuqvvlhqp3mqipt5@tkn_work_nb> (raw)
In-Reply-To: <2a06406f-3ea0-b892-5c8d-a0ee54e07d60@tarantool.org>

I would prefer if you will answer to review comments explicitly.

Re using lua_tolstring(): I understood that lua_tolstring() only
guarantees that a string is valid until it will be popped from a stack
(despite that fact that we know it has another copy on the stack) and so
a region usage looks good.

Just for the record: We discussed voicely that make this patch depending
on the luaT_tuple_new() implementation from the merger patchset is not
worth to do (just saves two lines of code here).

Changes I made (I mostly add them into a separate 'FIXUP' commit to
allow you to look into them if you want; pushed to your branch back):

* Squashed the commits, updated the commit message.
* Renamed :to_table() to totable().
* Use memcpy(tmp, path, path_len + 1) instead of an explicit '\0'
  assignment (lua_tolstring() allows it).
* Use 'struct lua_State' instead of just 'lua_State' to better match
  with our code style.
* Rewrote a test: split it by cases, do some renaming, fixed mine
  parts = {...} to parts = {{...}}, added more cases.
* Found an assertion fault with a new test case:

tarantool: /home/alex/p/tarantool-meta/r/tarantool/src/box/tuple_extract_key.cc:140: char* tuple_extract_key_slowpath(const tuple*, key_def*, uint32_t*) [with bool contains_sequential_parts = false; bool has_optional_parts = false; bool has_json_paths = false; uint32_t = unsigned int]: Assertion `field != NULL' failed.
Aborted

See the first commented test case. I think that we should allow a
fieldno in key_def that is above then a tuple length when the field is
nullable and raise an error otherwise (when it is not nullable). (I
guess that we can do it at least partially by setting
has_optional_parts).

However when I tried with non-nullable part with {fieldno = 2}
:extract_key({'foo'}) gives me {'foo', 80} -- it seems to go over its
memory. But it asserts like the first one if I added {fieldno = 3}.

All that cases are commented now. Can you please elaborate how to
archieve the behaviour I proposed above and whether it is good way to
go, Kirill?

Also I have an idea: maybe add ability to use tables as tuples? We have
to receive tuples in this way from net.box.

WBR, Alexander Turenko.

On Tue, Mar 26, 2019 at 04:37:54PM +0300, Kirill Shcherbatov wrote:
> Hi! Thank you for review.
> https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods
> https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110

  reply	other threads:[~2019-03-26 22:54 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-25 12:27 [tarantool-patches] " Kirill Shcherbatov
2019-03-26  1:46 ` [tarantool-patches] " Alexander Turenko
2019-03-26 13:37   ` Kirill Shcherbatov
2019-03-26 22:54     ` Alexander Turenko [this message]
2019-04-23 15:06       ` Konstantin Osipov
2019-04-23 15:09         ` Kirill Shcherbatov
2019-04-23 15:05 ` Konstantin Osipov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190326225408.tuqvvlhqp3mqipt5@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox