alamb commented on PR #5675:
URL:
https://github.com/apache/arrow-datafusion/pull/5675#issuecomment-1496306872
> The coercion rule that modifies sides of arithmetic op is not idempotent.
Multiple runs of the rule will change it to incorrect result. So we need
something to prevent the rule on coerced sides. PromotePrecision is such a
thing, it's just a wrapper for the purpose.
> For the new data_type on BinaryExpr. The coerced type of decimal
arithmetic op is not the same as the result type of it as you can see. So we
cannot simply take coerced type of left/right sides and use it as result type.
We cannot compute the result type on-the-fly in physical BinaryExec because it
depends on original datatypes of sides of the op, but we only have coerced at
the moment. So we need to record the result type so we can get it when
computing the decimal arithmetic result.
This makes sense -- thank you
My biggest concern with the approach in this PR is that it adds some special
case for decimal (which seems reasonable in itself) but that special case
bleeds out over the rest of the code / plans, which is probably why it got so
big.
I wonder would it be possible to use `PromotePrecision` only and avoid
modifying Expr::Binary to keep the behavior more localized to coercion.
Perhaps something like the following.
```rust
/// The target decimal type the expression should be output to
struct PromotePrecision {
expr: Box<Expr>,
precision: u8,
scale: u8,
}
```
One of the downsides of this approach is that PromotePrecision is now
clearly special purpose for Decimal. I am not sure if it would have other uses
in the future 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
