Re: [Tarantool-patches] [PATCH v2 10/15] module api: add box_key_def_new_v2()

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

On Sun, Oct 11, 2020 at 05:25:54PM +0200, Vladislav Shpilevoy wrote:
> 1. What if key_def/merger module is used on client tarantool? For example,
> vshard router. It does not store collation structs in the hash you use
> to lookup the name. Should we fix that on the clients, to make them fill
> the hash?
> 
> This kind of issue exists for all schema-dependent values. Mostly for SQL,
> which heavily depends on struct space/index/... even during parsing.

I don't know that we can do with it on this level.

It looks closer to https://github.com/tarantool/tarantool/issues/3982
('New cluster-wide replication group').

> > +	/* Set internal_part->path (JSON path). */
> > +	if (part->path) {
> 
> 2. Better use != NULL. At least to be consistent with the other code in
> in the patch.

Agree, just inattention.

> > +/**
> > + * Verify type and message of an error in the diagnostics area.
> > + *
> > + * Accepts a prefix of an actual type or a message. Pass an empty
> > + * string if you need to verify only type or only message.
> > + */
> > +static void
> > +check_diag(const char *exp_err_type, const char *exp_err_msg)
> > +{
> > +	box_error_t *e = box_error_last();
> > +	assert(!strcmp(box_error_type(e), exp_err_type));
> > +	assert(!strcmp(box_error_message(e), exp_err_msg));
> 
> 3. strcmp() returns not a boolean. Better check for == 0
> explicitly. Otherwise !strcmp(a, b) looks like a != b intuitively.
> In other patches too.

Agree, just inattention. Fixed over all patches.

> > +
> > +/**
> > + * Basic <box_key_part_def_create>() and <test_key_def_new_v2>()
> 
> 4. test_key_def_new_v2 -> box_key_def_new_v2.

Fixed (as well as the second such place).

> > +	/* Should not accept NULL as a <field_type>. */
> > +	key_def = box_key_def_new_v2(&part, 1);
> > +	assert(key_def == NULL);
> > +	check_diag("IllegalParams", "Field type is mandatory");
> 
> 5. Will you test not existing collation, unknown field type, bad JSON path?

It is already in my backlog: I would do it after the initial work.
Sorry, tight deadlines...

Even without bad cases, I spent a lot of time on tests already.