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

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

alamb commented on code in PR #5675:
URL: https://github.com/apache/arrow-datafusion/pull/5675#discussion_r1155962533


##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -39,6 +40,13 @@ pub trait ExprSchemable {
 
     /// cast to a type with respect to a schema
     fn cast_to<S: ExprSchema>(self, cast_to_type: &DataType, schema: &S) -> Result<Expr>;
+
+    /// promote to a type with respect to a schema

Review Comment:
   Can you please add some comments to clarify what the difference between "promote" and "cast" is? 



##########
datafusion/expr/src/expr.rs:
##########
@@ -385,6 +408,20 @@ impl Cast {
     }
 }
 
+/// Cast expression

Review Comment:
   Can we please add documentation about what `PromotePrecision` does and in what circumstances it is needed?



##########
datafusion/expr/src/expr.rs:
##########
@@ -234,12 +236,33 @@ pub struct BinaryExpr {
     pub op: Operator,
     /// Right-hand side of the expression
     pub right: Box<Expr>,
+    /// The data type of the expression, if known

Review Comment:
   I don't understand the reason for this change -- is it no longer possible to calculate the output type of a `BinaryExpr` from its inputs? 
   
   If the desired output type is different then couldn't we `cast` the expr to the type?



##########
datafusion/core/tests/sqllogictests/test_files/tpch.slt:
##########
@@ -125,7 +125,7 @@ select
     sum(l_quantity) as sum_qty,
     sum(l_extendedprice) as sum_base_price,
     sum(l_extendedprice * (1 - l_discount)) as sum_disc_price,
-    sum(l_extendedprice * (1 - l_discount) * (1 + l_tax)) as sum_charge,
+    sum(cast(l_extendedprice as decimal(12,2)) * (1 - l_discount) * (1 + l_tax)) as sum_charge,

Review Comment:
   perhaps we can add a comment here referencing the new arrow-rs kernel / work. Otherwise this context will likely get forgotten



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