> On 16 Apr 2019, at 17:12, Vladislav Shpilevoy wrote: > > On 14/04/2019 18:04, Nikita Pettik wrote: >> In most cases we don't assign and store type of node of expression AST >> (except for constant literals). To determine type of node we use >> sql_expr_type() function, which implements logic of quite simple >> recursive tree traversal. Before this patch we set type of node after >> code generation in sqlExprCodeTarget() without any traversal. This >> approach is way worse even then sql_expr_type(). > > Why? How is recursive scan of the tree better than simple access to > a struct's attribute? I see, that your patch improves type determination > precision, but I do not understand how, and what a perf price for that? Firstly, it seems that sqlExprCodeTarget() can be called on copy of original expr (when query contains order/group by clauses): I see that types assigned in that function are different from types in expr passed to generateColumnNames(). 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 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. >> 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. Ok, added label.