Re: [Tarantool-patches] [PATCH v5 01/52] sql: enhance vdbe_decode_msgpack_into_mem()

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Good job on the patch!

On 09.04.2021 18:51, Mergen Imeev via Tarantool-patches wrote:
> Hi! Thank you for the review! My answer and new patch below. I didn't include
> diffs in answers since due to merge conflicts they are partly useless.
> 
> On 30.03.2021 01:57, Vladislav Shpilevoy wrote:
>> Hi! I appreciate the work you did here!
>>
>> Truly huge patch. But it does the important thing which I think
>> should give a huge help in the task of SQL code rework.
>>
>> On 23.03.2021 10:34, Mergen Imeev via Tarantool-patches wrote:
>>> Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not
>>> properly initialized in case msgpack contains MP_EXT, MP_MAP, or
>>> MP_ARRAY fields. Also, it doesn't set field_type.
>>
>> AFAIR, field type wasn't set deliberately. Because we use it only for
>> strictly typed values obtained from formatted space fields. It wasn't
>> applied to plain non-formatted values on purpose.
>>
>> Why do you change that?
> 
> I didn't know about that. I thought that all MEMs that contains data should have
> field_type set. However, I tried to understand where did this field come from
> and found that it was added for two purposes: to show difference between NUMBER
> and INTEGER in MEM before DOUBLE was added and to show column name instead of
> type determined from MP-type in typeof(). I believe that both these purposes are
> not needed now and that this field should be removed from struct MEM. I created
> an issue for this: #5957. However, I was prohibited to remove this field for now
> by @tsafin, who believes that this field is actually important.

I tend to agree, that it must be done separately. Although probably could be done
before this patchset to make it kind of simpler. Anyway, thanks for the
explanation.