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

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Sun Apr 11 20:42:22 MSK 2021


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.


More information about the Tarantool-patches mailing list