From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 25 Sep 2018 13:11:04 +0300 From: Kirill Yukhin Subject: Re: [PATCH] sql: do not store key_def in VDBE op Message-ID: <20180925101104.scokyq4wwluive4x@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: Hello, On 24 сен 20:49, Vladimir Davydov wrote: > The parser must not deal with internal Tarantool objects, such as space, > index, or key_def, directly, because it (a) violates encapsulation, > turning the code into a convoluted mess, and (b) makes it impossible to > run the parser and VDBE on different instances, which might come in > handy for cluster SQL implementation. Instead, it should store plain > names in the generated VDBE code. It may use objects the sole purpose > of which is to represent object definitions, such as key_part_def or > field_def, though. > > This patch does a tiny step in this direction. It replaces key_def with > sql_key_info in VDBE arguments. The new structure is a trivial wrapper > around an array of key_part_def. It is ref-countable, just like its > predecessor KeyInfo, so as to avoid unnecessary memory duplication. > Since key_def spread roots deeply into the parser implementation, the > new structure has two extra methods: > > sql_key_info_new_from_key_def > sql_key_info_to_key_def > > so that it can be converted to/from a key definition. Note, the latter > caches the result so as not to create a new key definition on subsequent > function calls. > > This partially undoes the work done by commit 501c6e28b095 ("sql: > replace KeyInfo with key_def"). > > The reason why I'm doing this now is that I want to dispose of the > key_def_set_part function, which is used extensively by the parser, > because the latter stores key_def directly in VDBE op. This function > has a vague semantic and rather obscures construction of a key > definition. It will get especially nasty once JSON path indexes are > introduced in Tarantool core. The new struct, sql_key_info, allows us > to get rid of most of those calls. > --- > https://github.com/tarantool/tarantool/tree/merge-2.0 Your patch is OK for 2.0 branch. -- Regards, Kirill Yukhin