From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 DD01D46970E for ; Sat, 25 Jan 2020 10:04:56 +0300 (MSK) From: Chris Sosnin Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_38C17265-AF1C-4DC6-B3BE-9224555CB821" Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\)) Date: Sat, 25 Jan 2020 10:04:55 +0300 In-Reply-To: <77348412-c9ac-54e7-2997-f9b2a2a9e0cb@tarantool.org> References: <20200119201815.71286-1-k.sosnin@tarantool.org> <77348412-c9ac-54e7-2997-f9b2a2a9e0cb@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] box: add binary search for _session_settings space List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org --Apple-Mail=_38C17265-AF1C-4DC6-B3BE-9224555CB821 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thank you for the review and your fixes! See my answers below. > 3. I would rather say to '*an* existing'. Because there is no a one > certain setting to which all the iterators point after setting that > flag. Thank you, I agree. > 4. Even though that bsearch passes the tests, I failed to understand > it. Please, consider a more canonical bsearch on the branch in a > separate commit. Feel free to comment, if someone catches your = attention > in my version and you don't agree. Your version is better, I agree, my code just implements lower/upper = bound, but there is no point in it, because all settings are unique. Thanks! >> + if (low < count) { >> + int cmp =3D strcmp(name[low], key); >> + if ((cmp =3D=3D 0 && is_including) || >> + (cmp > 0 && !is_eq)) { >> + *sid =3D low; >> + return 0; >> + } >> + } >=20 > 5. This last strcmp can be avoided. Or at least > moved into the cycle. See my commit. >=20 > All the same about 'reverse' version. I believe with your fixes it doesn=E2=80=99t really matter anymore. >=20 > 6. What I don't like here is that we check is_set as many times, > as many modules we have. Moreover, if set_forward didn't find > anything, you fallback to a fullscan in that module, even though > it wont find anything with 100% guarantee. >=20 > I fixed it, see my commit. The thing is that if set_forward fails, then sid =3D count and = next_in_module will immediately return -1. But if we are going to remove modules, then = this part will be rewritten anyway. > I fixed settings tests also - there was no a check that > real settings space, and a user defined one return the same tuple = count. It > could happen, that one of them returns 0 tuples, and it would be = considered > success. >=20 > Additionally, I added a test, which tries all existing iterators on = every > setting. Thanks, I agree with those. --Apple-Mail=_38C17265-AF1C-4DC6-B3BE-9224555CB821 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

Hi! Thank you for the review and your fixes!

See my answers below.

3. I would = rather say to '*an* existing'. Because there is no a one
certain setting to which all the = iterators point after setting that
flag.

Thank = you, I agree.

4. Even though that bsearch passes the tests, I failed to = understand
it. Please, = consider a more canonical bsearch on the branch in a
separate commit. Feel free to = comment, if someone catches your attention
in my version and you don't agree.

Your = version is better, I agree, my code just implements lower/upper = bound,

but there is no point in it, because all settings are = unique. Thanks!

+ if (low = < count) {
+ int cmp =3D strcmp(name[low], = key);
+ if ((cmp =3D=3D 0 && is_including) ||
+ = = (cmp > = 0 && !is_eq)) {
+ *sid =3D low;
+ return = 0;
+ }
+ }

5. This last strcmp can be = avoided. Or at least
moved into the cycle. See my commit.

All the same about 'reverse' = version.

I believe with your fixes it doesn=E2=80=99t = really matter anymore.

6. What I don't like here is that we check is_set as many = times,
as many = modules we have. Moreover, if set_forward didn't find
anything, you fallback to a = fullscan in that module, even though
it wont find anything with 100% guarantee.

I fixed it, see my = commit.

The thing = is that if set_forward fails, then sid =3D count and = next_in_module

will immediately return -1. But if we are going = to remove modules, then this

part will be rewritten = anyway.

I fixed settings tests also - there was no a check = that
real settings = space, and a user defined one return the same tuple count. It
could happen, that one of them = returns 0 tuples, and it would be considered
success.

Additionally, I added a test, = which tries all existing iterators on every
setting.

Thanks, I agree = with those.

= --Apple-Mail=_38C17265-AF1C-4DC6-B3BE-9224555CB821--