From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 25 Apr 2019 16:32:27 +0300 From: Alexander Turenko Subject: Re: [tarantool-patches] [PATCH v3 6/7] Add merger for tuples streams (C part) Message-ID: <20190425133227.mvell3bwcbhzqkyb@tkn_work_nb> References: <963ad528ad35199943931150956c1d5e2c374c40.1554906327.git.alexander.turenko@tarantool.org> <20190425114343.GM29257@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190425114343.GM29257@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org, Vladimir Davydov List-ID: On Thu, Apr 25, 2019 at 02:43:43PM +0300, Konstantin Osipov wrote: > * Alexander Turenko [19/04/10 18:23]: > > +enum { MERGER_SOURCE_REF_MAX = INT_MAX }; > > It's merge_source, not merger, source, no? Here and in the rest of > the code. merge source or merger's source. I found it neat to have one prefix 'merger_' for all related structures and functions. But okay, I'll check it with Vladimir and if he agree will change those names. > > > +void > > +merger_context_delete(struct merger_context *ctx) > > Please decide it's either simply merger or merge_context. > merger_context does not make any sense to me. merger is a special kind of a source, the different thing. > > what's in the context anyway? Can you have multiple contexts in a > single merge? If no, why not simply merger? Why do you need both, > a merger and a context? > > > +/** > > + * Holds immutable parameters of a merger. > > + */ > > +struct merger_context { > > + struct key_def *key_def; > > + box_tuple_format_t *format; > > + /* Reference counter. */ > > + int refs; > > +}; > > Uhm.. Is it a comparison/sort context, not merger context then? key_def is here for comparisons, yep, but format is for creating tuples (with needed offsets). I'm like 'merger_' prefix, because it makes clear that it is something that a merger need to perform its work. The name is very common, yep. 'Comparison/sort context' would be more specific, but not exact: it would arise a question: 'why a format is a part of comparison context'? I'll discuss this point with Vladimir too. > Please explain why you need to ref it btw. I would simply pass it > into Lua via ffi, to avoid any references, and make it clear that > the life cycle of this object is bound to the merge window. So you'll need to construct a format in Lua somehow. Moreover, you'll need to create *right* format in Lua to make comparisons fast. Looks as a way more complex approach. Maybe I didn't get your idea.