[tarantool-patches] Re: [PATCH v8.5] sql: add index_def to struct Index

[Vladislav Shpilevoy]

Hello. Thanks for the fixes!

On 03/07/2018 12:46, Ivan Koptelov wrote:
> Thank for the review. Previous mail contains patch with an error. Here it is fixed.
>> Hello. Thanks for the fixes! See my 6 comments below.
>>
>> And I have pushed more fixes on the branch. Please,
>> look and squash.
>>
>>> Now every sqlite struct Index is created with tnt struct
>>> index_def inside. This allows us to use tnt index_def
>>> in work with sqlite indexes in the same manner as with
>>> tnt index and is a step to remove sqlite Index with
>>> tnt index.
>>> Fields coll_array, coll_id_array, aiColumn, sort_order
>>> and zName are removed from Index. All usages of this
>>> fields changed to usage of corresponding index_def
>>> fields.
>>> index_is_unique(), sql_index_collation() and
>>> index_column_count() are removed with calls of
>>> index_def corresponding fields.
>>>
>>> Closes: #3369
>>>
>>> ---
>>> Branch:
>>> https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where
>>> Issue:https://github.com/tarantool/tarantool/issues/3369
>>>
>>>   src/box/sql.c                        |  54 ++-
>>>   src/box/sql/analyze.c                |  85 ++---
>>>   src/box/sql/build.c                  | 713 +++++++++++++++++------------------
>>>   src/box/sql/delete.c                 |  10 +-
>>>   src/box/sql/expr.c                   |  61 +--
>>>   src/box/sql/fkey.c                   | 132 +++----
>>>   src/box/sql/insert.c                 | 145 ++++---
>>>   src/box/sql/pragma.c                 |  30 +-
>>>   src/box/sql/select.c                 |   2 +-
>>>   src/box/sql/sqliteInt.h              | 111 ++----
>>>   src/box/sql/update.c                 |  39 +-
>>>   src/box/sql/vdbeaux.c                |   2 +-
>>>   src/box/sql/vdbemem.c                |  21 +-
>>>   src/box/sql/where.c                  | 180 ++++-----
>>>   src/box/sql/wherecode.c              | 102 ++---
>>>   test/sql-tap/colname.test.lua        |   4 +-
>>>   test/sql/message-func-indexes.result |   8 +-
>>>   17 files changed, 821 insertions(+), 878 deletions(-)
>>>
>>> -
>>> -/** Return true if given index is unique and not nullable. */
>>> -bool
>>> -index_is_unique_not_null(const Index *idx)
>>
>> 4. Same as on the previous review. Still is used in a pair of places.
> Are you sure? I searched through the whole project and didn't find it.
> There is only a variable with the same name in one place.

Sorry, my fault. It is just a variable.

> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 85143ed20..7ca02095f 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -3058,6 +3064,8 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder,	/* WHERE clause information */
>   		if (pSrc->pIBIndex)
>   			break;
>   	}
> +	if (&sPk == pProbe && sPk.def != NULL)
> +		index_def_delete(sPk.def);

It will not be deleted since pProbe is changed in the cycle above.
I have checked it by putting assert(false) under this 'if'.
And I have already fixed it on the branch.

>   	return rc;
>   }
>   

I have pushed my last fixes on the branch. Please, look, squash and then
LGTM. Nikita, please, review after the squash.