Re: [tarantool-patches] [PATCH v3 6/7] Add merger for tuples streams (C part)

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Thu, Apr 25, 2019 at 02:43:43PM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [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.