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