From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id D7817469719
 for <tarantool-patches@dev.tarantool.org>;
 Mon, 12 Oct 2020 10:20:59 +0300 (MSK)
Date: Mon, 12 Oct 2020 10:21:18 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201012072118.bwl4euhzd6exzz4j@tkn_work_nb>
References: <cover.1602420460.git.alexander.turenko@tarantool.org>
 <fd57788d18f20f570d17013d686ad3adddf86f2b.1602420460.git.alexander.turenko@tarantool.org>
 <ef8d2cf0-9c2b-6d04-269f-80a3b670f96c@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ef8d2cf0-9c2b-6d04-269f-80a3b670f96c@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v2 10/15] module api: add
 box_key_def_new_v2()
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.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.