[tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure

[Vladislav Shpilevoy]

LGTM. Nikita, please, review.

On 18/07/2018 10:25, Kirill Shcherbatov wrote:
>> 1. Please, rebase on the latest 2.0 and use pk->def + key_def_find.
> -       if (index_has_column(pk, pNew->def->field_count - 1)) {
> +       assert(pk != NULL);
> +       struct key_def *pk_key_def = pk->def->key_def;
> +       if (key_def_find(pk_key_def, pNew->def->field_count - 1) != NULL) {
> 
>> 2. As I said verbally, you should not add this redundant scan of primary
>> index columns. You already have the primary index scan in
>> convertToWithoutRowidTable, that is called few lines above.
>>
>> When you merge this scan into convertToWithoutRowidTable, you can
>> inline the rest of the function into EndTable and remove it together
>> with index_has_column.
> I can't understand your suggestion.
> convertToWithoutRowidTable iterates by index field;
> but this check is a about field with ON CONFLICT REPLACE could be only a part
> of index. We iterate over all fields.
> 
> I've get rid off index_has_column.
> 
>                  if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
> -                   !index_has_column(pk, i))
> +                   (pk == NULL || key_def_find(pk->def->key_def, i) == NULL))
> 
> 
> -bool
> -index_has_column(struct Index *index, uint32_t  column_idx)
> etc.
>