<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 16 Apr 2019, at 17:12, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" class="">v.shpilevoy@tarantool.org</a>> wrote:</div><div class=""><div class=""><br class="">On 14/04/2019 18:04, Nikita Pettik wrote:<br class=""><blockquote type="cite" class="">In most cases we don't assign and store type of node of expression AST<br class="">(except for constant literals). To determine type of node we use<br class="">sql_expr_type() function, which implements logic of quite simple<br class="">recursive tree traversal. Before this patch we set type of node after<br class="">code generation in sqlExprCodeTarget() without any traversal. This<br class="">approach is way worse even then sql_expr_type().<br class=""></blockquote><br class="">Why? How is recursive scan of the tree better than simple access to<br class="">a struct's attribute? I see, that your patch improves type determination<br class="">precision, but I do not understand how, and what a perf price for that?<br class=""></div></div></blockquote><div><br class=""></div><div>Firstly, it seems that sqlExprCodeTarget() can be called on copy of</div><div>original expr (when query contains order/group by clauses): I see</div><div>that types assigned in that function are different from types in expr</div><div>passed to generateColumnNames().</div><div><br class=""></div>Secondly, suggested approach simply makes code cleaner and easier</div><div>to maintain: we shouldn’t care about adding on each branch of switch</div><div>appropriate type (since it is already done in sql_expr_type()).</div><div><br class=""></div><div>Eventually, sqlExprCodeTarget() doesn’t take into consideration</div><div>types of child nodes. This drawback will appear when we extend</div><div>integer type with small/big ints:</div><div><br class=""></div><div>Column a is of type smallint.</div><div><br class=""></div><div>SELECT 1 + a;</div><div><br class=""></div><div>Should result in ‘smallint' type, not common ‘number’.</div><div>Without tree traversal we can’t do that.<br class=""><div><br class=""></div><div>Price is a primitive traversal through the expr tree. This traversal (but in more</div><div>sophisticated implementation and being called only once saving in each node</div><div>its type) is anyway required to implement this task:</div><div><a href="https://github.com/tarantool/tarantool/issues/3103" class="">https://github.com/tarantool/tarantool/issues/3103</a></div><div><br class=""></div><div>In fact, we should once visit each node (probably after names resolution)</div><div>and save its type depending on child’s types.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">If you are not sure about the price, we could ask Alexander to run<br class="">benches on your branch before pushing into the master.</div></div></blockquote><div><br class=""></div><div>This patch is auxiliary to main-patchset. I’ve added it to avoid</div><div>abusing changing result files like sql/types.result etc. Can drop it tho.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class="">So, to improve accuracy<br class="">of type determination, let's always call that method and remove type<br class="">assignment in sqlExprCodeTarget().<br class=""></blockquote><br class="">This patch fixes the issue<br class=""><a href="https://github.com/tarantool/tarantool/issues/4126" class="">https://github.com/tarantool/tarantool/issues/4126</a>.<br class=""></div></div></blockquote><br class=""></div><div>Ok, added label.</div><br class=""></body></html>