>> 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 > > > 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 . >> >> 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.