Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Ilya Kosarev <i.kosarev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] key_def: support composite types extraction
Date: Fri, 25 Sep 2020 21:34:50 +0300	[thread overview]
Message-ID: <20200925183450.atsuxr4ine7c2dv7@tkn_work_nb> (raw)
In-Reply-To: <20200921121100.10052-1-i.kosarev@tarantool.org>

I have no objections in general, but there are doubts around several
places. Please, look below.

WBR, Alexander Turenko.

> +static bool
> +key_def_comparable(struct key_def *key_def)

What make me doubt: key_def is not comparable per se, it may or may not
be used for comparison of tuples and a tuple with a key.
'key_def_has_comparator' or 'key_def_can_compare' (however not key_def
itself perform comparisons, hmm), maybe, don't know.

> +{
> +	for (uint32_t i = 0; i < key_def->part_count; ++i) {
> +		if (key_def->parts[i].type == FIELD_TYPE_ANY ||
> +		    key_def->parts[i].type == FIELD_TYPE_ARRAY ||
> +		    key_def->parts[i].type == FIELD_TYPE_MAP) {
> +			/* Tuple comparators don't support these types. */
> +			diag_set(IllegalParams, "Unsupported field type: %s",
> +				 field_type_strs[key_def->parts[i].type]);
> +			return false;
> +		}
> +	}
> +	return true;
> +}
> +

Ilya gives the idea: perform this check on key_def creation and store a
flag inside key_def. Check against the flag in lbox_key_def_compare()
and lbox_key_def_compare_with_key().

This looks as the right way to solve this kind of problems: comparisons
are more hot functions than key_def creation.

We can sink it down to key_def_set_compare_func() and set NULL to
key_def->{tuple_compare,tuple_compare_with_key}. Than check it in
lbox_key_def_compare*() and add asserts to tuple_compare*(). No new
fields will be required so.

This part surely should look someone, who is more near to comparators
than me.

>  /**
>   * Free a key_def from a Lua code.
>   */
> @@ -316,6 +320,9 @@ lbox_key_def_compare(struct lua_State *L)
>  				     "compare(tuple_a, tuple_b)");
>  	}
>  
> +	if (!key_def_comparable(key_def))
> +		return luaT_error(L);
> +
>  	struct tuple *tuple_a, *tuple_b;
>  	if ((tuple_a = luaT_key_def_check_tuple(L, key_def, 2)) == NULL)
>  		return luaT_error(L);
> @@ -349,6 +356,9 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>  				     "compare_with_key(tuple, key)");
>  	}
>  
> +	if (!key_def_comparable(key_def))
> +		return luaT_error(L);
> +
>  	struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);
>  	if (tuple == NULL)
>  		return luaT_error(L);
> diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
> index 3a4aad68721..8fcdf7070bf 100755

How about lbox_key_def_merge() and underlying functions? I'm not sure
they will work correct. At least I tried this on the branch:

 | tarantool> key_def = require('key_def')
 | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'array'}})
 | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'map'}})
 | tarantool> kd1:merge(kd2)
 | ---
 | - - type: array
 |     is_nullable: false
 |     fieldno: 1
 | ...

It does not look correct.

Everything looks good with lbox_key_def_to_table(), but I would add a
test anyway.

  reply	other threads:[~2020-09-25 18:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-21 12:11 Ilya Kosarev
2020-09-25 18:34 ` Alexander Turenko [this message]
2020-09-26 21:53   ` Ilya Kosarev
2020-10-01 11:38     ` Alexander Turenko
2020-10-01 15:26       ` Ilya Kosarev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200925183450.atsuxr4ine7c2dv7@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=i.kosarev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] key_def: support composite types extraction' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox