[tarantool-patches] Re: [PATCH 5/9] sql: improve type determination for column meta

[n.pettik]

>> Secondly, suggested approach simply makes code cleaner and easier
>> to maintain: we shouldn’t care about adding on each branch of switch
>> appropriate type (since it is already done in sql_expr_type()).
>> 
>> Eventually, sqlExprCodeTarget() doesn’t take into consideration
>> types of child nodes. This drawback will appear when we extend
>> integer type with small/big ints:
>> 
>> Column a is of type smallint.
>> 
>> SELECT 1 + a;
>> 
>> Should result in ‘smallint' type, not common ‘number’.
>> Without tree traversal we can’t do that.
>> 
>> Price is a primitive traversal through the expr tree. This traversal (but in more
>> sophisticated implementation and being called only once saving in each node
>> its type) is anyway required to implement this task:
>> https://github.com/tarantool/tarantool/issues/3103 <https://github.com/tarantool/tarantool/issues/3103> <https://github.com/tarantool/tarantool/issues/3103 <https://github.com/tarantool/tarantool/issues/3103>>
> 
> I do not have anything against traversal. I just don't like multiple
> traversal and seemingly free and fast call of deceptive sql_expr_type() -
> it is expensive in fact, just look at this huge 'switch-case'.
> 
> Your patch is ok, it does not aggravate anything. But sql_expr_type()
> is called in very many places. Probably, in scope of 3103 we should
> set Expr type always before optimizer and Vdbe-coder are started. Just
> thoughts.
> 
>> In fact, we should once visit each node (probably after names resolution)
>> and save its type depending on child’s types.
>> 
>>> If you are not sure about the price, we could ask Alexander to run
>>> benches on your branch before pushing into the master.
>> 
>> This patch is auxiliary to main-patchset. I’ve added it to avoid
>> abusing changing result files like sql/types.result etc. Can drop it tho.
> 
> Now I see, that there is nothing to bench. Almost pure refactoring.
> 
>>>> So, to improve accuracy
>>>> of type determination, let's always call that method and remove type
>>>> assignment in sqlExprCodeTarget().
>>> 
>>> This patch fixes the issue
>>> https://github.com/tarantool/tarantool/issues/4126 <https://github.com/tarantool/tarantool/issues/4126>.
>> 
>> Ok, added label.
>> 
> 
> During the second review I found some orphan type assignments
> in codetarget(). I dropped them and the tests passed. The diff is
> on the branch.

Thx, applied as obvious.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190423/a04ff961/attachment.html>