From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Mon, 11 Mar 2019 13:39:58 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v5 2/4] memtx: introduce tuple compare hint
Message-ID: <20190311103958.2zh6cdzbqnpzjrfa@esperanza>
References: <cover.1551951540.git.kshcherbatov@tarantool.org>
 <53e165cf566611452821c692d79d621628e839ff.1551951540.git.kshcherbatov@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <53e165cf566611452821c692d79d621628e839ff.1551951540.git.kshcherbatov@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Thu, Mar 07, 2019 at 12:44:06PM +0300, Kirill Shcherbatov wrote:
> +void
> +key_def_set_cmp_aux_func(struct key_def *def)
> +{
> +	def->key_cmp_aux = key_hint_default;
> +	def->tuple_cmp_aux = tuple_hint_default;
> +	bool is_nullable = key_part_is_nullable(def->parts);
> +	if (def->parts->type == FIELD_TYPE_STRING && def->parts->coll != NULL) {
> +		def->key_cmp_aux = is_nullable ? key_hint_string_coll<true> :
> +						 key_hint_string_coll<false>;
> +		def->tuple_cmp_aux = is_nullable ?
> +				     tuple_hint_string_coll<true> :
> +				     tuple_hint_string_coll<false>;
> +		return;
> +	}
> +	switch (def->parts->type) {
> +	case FIELD_TYPE_UNSIGNED:
> +		def->key_cmp_aux = is_nullable ? key_hint_uint<true> :
> +						 key_hint_uint<false>;
> +		def->tuple_cmp_aux = is_nullable ? tuple_hint_uint<true> :
> +						   tuple_hint_uint<false>;
> +		break;
> +	case FIELD_TYPE_INTEGER:
> +		def->key_cmp_aux = is_nullable ? key_hint_int<true> :
> +						 key_hint_int<false>;
> +		def->tuple_cmp_aux = is_nullable ? tuple_hint_int<true> :
> +						   tuple_hint_int<false>;
> +		break;
> +	case FIELD_TYPE_STRING:
> +		def->key_cmp_aux = is_nullable ? key_hint_string<true> :
> +						 key_hint_string<false>;
> +		def->tuple_cmp_aux = is_nullable ? tuple_hint_string<true> :
> +						   tuple_hint_string<false>;
> +		break;
> +	case FIELD_TYPE_NUMBER:
> +		def->key_cmp_aux = is_nullable ? key_hint_number<true> :
> +						 key_hint_number<false>;
> +		def->tuple_cmp_aux = is_nullable ? tuple_hint_number<true> :
> +						   tuple_hint_number<false>;
> +		break;
> +	case FIELD_TYPE_BOOLEAN:
> +		def->key_cmp_aux = is_nullable ? key_hint_boolean<true> :
> +						 key_hint_boolean<false>;
> +		def->tuple_cmp_aux = is_nullable ? tuple_hint_boolean<true> :
> +						   tuple_hint_boolean<false>;
> +		break;
> +	default:
> +		break;
> +	};
>  }

I haven't looked into this patch in details yet (expect a separate
email), but at the first glance it looks there's a serious problem we
must address before moving forward - you don't take into account SCALAR
type. The tricky part is SCALAR can be converted to any other basic type
(e.g. INTEGER) without rebuilding the index. So we should either
introduce hints for SCALAR types as well and make them compatible with
basic types or disable hints for SCALAR altogether. Anyway, we need a
test for this.