[GitHub] [arrow-datafusion] alamb commented on pull request #5675: Decimal multiply kernel should not cause precision loss

["alamb (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org