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 v2] key_def: support composite types extraction
Date: Thu, 1 Oct 2020 14:47:44 +0300	[thread overview]
Message-ID: <20201001114744.yxzei3orpoqoji2k@tkn_work_nb> (raw)
In-Reply-To: <20200926215307.15808-1-i.kosarev@tarantool.org>

LGTM.

I see a bit different way to do the same, but I don't know whether it is
better. So I would say that both ways are okay for me, but it would be
good if someone other would look into this discussion too. I hope
Alexander L. will do.

See also the comment regarding the dubious test case.

WBR, Alexander Turenko.

> +	/**
> +	 * First part type not supported for comparison.
> +	 * Valid if key_def does not support comparison
> +	 * (key_def->tuple_compare* == NULL), undefined othewise.

I would set it to field_type_MAX in the case, but maybe it is just my
fear of undefined values.

> +	 */
> +	enum field_type unsupported_type;

I see the alternative (but I don't know whether it is better). We can
wrap the loop into a function and use it directly on the error path
(when key_def->tuple_compare or key_def->tuple_compare_with_key is NULL)
to obtain an error message. A performance of the error path is not
important I guess. This way we'll not need the extra field in <struct
key_def>.

Unlikely we'll create a lot of key_defs, so the question is purely how
the code would look better. A field just to pass it to an error message
is a bit artificial thing, IMHO.

Let the check function (key_def_has_comparator() or so) to set the
diagnostics and so the checking code and the construction of an error
message will be in one place. The usage would be like so:

In key_def_set_compare_func():

 | if (! key_def_has_comparator(def)) {
 |         def->tuple_compare = NULL;
 |         def->tuple_compare_with_key = NULL;
 | }

In lbox_key_def_compare():

 | if (key_def->tuple_compare == NULL) {
 |         /* Just to set an error to the diagnostics area. */
 |         int rc = key_def_has_comparator(key_def);
 |         (void) rc;
 |         assert(rc != 0);
 |         return luaT_error(L);
 | }

But I'll repeat: I don't know whether it worth to do. Feel free to
ignore.

> +	if (key_def->tuple_compare == NULL) {
> +		diag_set(IllegalParams, "Unsupported field type: %s",
> +			 field_type_strs[key_def->unsupported_type]);
> +		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 +343,12 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>  				     "compare_with_key(tuple, key)");
>  	}
>  
> +	if (key_def->tuple_compare_with_key == NULL) {
> +		diag_set(IllegalParams, "Unsupported field type: %s",
> +			 field_type_strs[key_def->unsupported_type]);
> +		return luaT_error(L);
> +	}
> +
>  	struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);
>  	if (tuple == NULL)
>  		return luaT_error(L);

(Left here for context.)

> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index d059c709f..762bc8019 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -2081,5 +2081,16 @@ key_def_set_compare_func(struct key_def *def)
>  			key_def_set_compare_func_json<false, false>(def);
>  		}
>  	}
> +	for (uint32_t i = 0; i < def->part_count; ++i) {
> +		if (def->parts[i].type == FIELD_TYPE_ANY ||
> +		    def->parts[i].type == FIELD_TYPE_ARRAY ||
> +		    def->parts[i].type == FIELD_TYPE_MAP) {
> +			/* Tuple comparators don't support these types. */
> +			def->tuple_compare = NULL;
> +			def->tuple_compare_with_key = NULL;
> +			def->unsupported_type = def->parts[i].type;
> +			break;
> +		}
> +	}
>  	key_def_set_hint_func(def);
>  }

(Left here for context.)

> @@ -488,6 +525,35 @@ test:test('merge()', function(test)
>          'case 3: verify with :totable()')
>      test:is_deeply(key_def_cb:extract_key(tuple_a):totable(),
>          {1, 1, box.NULL, 22}, 'case 3: verify with :extract_key()')
> +
> +    local parts_unsigned = {
> +        {type = 'unsigned', fieldno = 1, is_nullable = false},
> +    }
> +    local key_def_unsigned = key_def_lib.new(parts_unsigned)
> +    local key_def_string = key_def_lib.new({
> +        {type = 'string', fieldno = 1},
> +    })
> +    local key_def_array = key_def_lib.new({
> +        {type = 'array', fieldno = 1},
> +        {type = 'unsigned', fieldno = 2},
> +    })
> +    local key_def_map = key_def_lib.new({
> +        {type = 'map', fieldno = 3, is_nullable = true},
> +        {type = 'scalar', fieldno = 2},
> +    })
> +
> +    local key_def_unsigned_string = key_def_unsigned:merge(key_def_string)
> +    test:is_deeply(key_def_unsigned_string:totable(), parts_unsigned,
> +        'in case of conflict we just get the field from the first key_def')

If you add a test case and not sure whether corrsponding behaviour is
right, there are two options. Either clarify that you just hold current
behaviour and presence of this test case does not mean that the
behaviour should remain the same in future (but what is purpose of the
case so?). Or don't add it.

Just to avoid any possible confusion like 'we test the behaviour, so it
seems there is some commitment that we'll keep it'.

I shared my doubts about this behaviour in [1].

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019807.html

  reply	other threads:[~2020-10-01 11:47 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-26 21:53 Ilya Kosarev
2020-10-01 11:47 ` Alexander Turenko [this message]
2020-10-01 18:35   ` 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=20201001114744.yxzei3orpoqoji2k@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=i.kosarev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2] 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