From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B4AE3215E4 for ; Mon, 16 Jul 2018 21:22:37 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id YpzX4IZIcwth for ; Mon, 16 Jul 2018 21:22:37 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CA191215B3 for ; Mon, 16 Jul 2018 21:22:36 -0400 (EDT) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_FF8E5B57-7C89-48DB-8FB3-009181C97804" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2] sql: add index_def to struct Index Date: Tue, 17 Jul 2018 04:22:28 +0300 In-Reply-To: References: <35c7947b-cb54-bac8-fed2-680c65f76cb6@tarantool.org> <0566bdde-5972-6c5e-707d-0ad58b6b45da@tarantool.org> <513397CB-4A4B-4C7D-A0C9-249000100E03@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Ivan Koptelov --Apple-Mail=_FF8E5B57-7C89-48DB-8FB3-009181C97804 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 I have pushed some fixes and refactoring on branch. Please, look at them and squash, if you are agree with them. In case you disagree or have doubts - reply to this letter with your comments.=20 If they are OK to you, patch LGTM. (Btw, you forgot to squash your last fixes with whole patch.) >> I don=E2=80=99t understand: why do you need at all this cmp_def? >> In SQL it is extremely unlikely to be useful. > SQL has memtx/vinyl under it, where cmp_def is used, isn't it? > As far as I understand, there is some mechanism, which creates > struct index with struct index_def inside memtx (for example, for > memtx_tree_create()) based on what we passed inside in > createIndex() in sql/build.c > Is it right? I am confused. > If it is right, then we can simplify code in build by always=20 > using key_def for cmp_def ? These defs are stored only in our SQL hash. After tuple is inserted into = _index, the real key_def and cmp_def are created under the hood. And those defs are used (see on_replace_dd_index()) in Tarantool's core. So, I basically removed all ceremony with cmp_def during index building; also I noticed that now only PK can feature ON REPLACE conflict action, thus I simplified addIndexToTable routine.=20 --Apple-Mail=_FF8E5B57-7C89-48DB-8FB3-009181C97804 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 I have pushed some fixes and refactoring on branch.
Please, look at them and squash, if you are agree with = them.
In case you disagree or have doubts - reply = to this letter with
your comments. 

If they are OK to you, = patch LGTM.
(Btw, you forgot to squash your last = fixes with whole patch.)

I don=E2=80=99t understand: why do you need at all this = cmp_def?
In SQL it is extremely unlikely to be = useful.
SQL has memtx/vinyl under it, where cmp_def is used, isn't = it?
As far as I = understand, there is some mechanism, which creates
struct index with struct index_def inside memtx = (for example, for
memtx_tree_create()) = based on what we passed inside in
createIndex() in sql/build.c
Is it right? I am confused.
If it is right, then we can simplify code in build by = always 
using key_def for cmp_def ?

These defs = are stored only in our SQL hash. After tuple is inserted into = _index,
the real key_def and cmp_def are created under the = hood. And those
defs are used (see on_replace_dd_index()) in = Tarantool's core.

So, I basically = removed all ceremony with cmp_def during index building;
also = I noticed that now only PK can feature ON REPLACE conflict = action,
thus I simplified addIndexToTable = routine. 


= --Apple-Mail=_FF8E5B57-7C89-48DB-8FB3-009181C97804--