kumarUjjawal commented on code in PR #22697:
URL: https://github.com/apache/datafusion/pull/22697#discussion_r3339113024
##########
datafusion/functions/src/math/round.rs:
##########
@@ -467,7 +506,36 @@ fn round_columnar(
&& matches!(decimal_places, ColumnarValue::Scalar(_));
let decimal_places_is_array = matches!(decimal_places,
ColumnarValue::Array(_));
+ macro_rules! round_integer_array_to_float64 {
+ ($ARRAY_TYPE:ty) => {{
+ let result = calculate_binary_math::<$ARRAY_TYPE, Int32Type,
Float64Type, _>(
+ value_array.as_ref(),
+ decimal_places,
+ |v, dp| round_float(v as f64, dp),
+ )?;
+ result as _
+ }};
+ }
+
let arr: ArrayRef = match (value_array.data_type(), return_type) {
+ (input_type, return_type)
Review Comment:
The no-op fast path requires `decimal_places` to be a non-negative scalar
literal. When the scale is a *column* (or a negative literal), this guard is
false and there's no other integer arm, so it hits `exec_err!`
```SQL
CREATE TABLE t(v BIGINT, dp INT) AS VALUES (125,1),(125,-1);
SELECT round(v, dp) FROM t; -- errored; worked (Float64) before this PR
```
##########
datafusion/functions/src/math/round.rs:
##########
@@ -308,6 +324,29 @@ impl ScalarUDFImpl for RoundFunc {
};
match (value_scalar, args.return_type()) {
+ (value_scalar, return_type)
+ if is_integer_data_type(return_type) && dp >= 0 =>
Review Comment:
only covers `dp >= 0`. For `dp < 0` with an integer value, needs handler for
negative scale
##########
datafusion/functions/src/math/round.rs:
##########
@@ -308,6 +324,29 @@ impl ScalarUDFImpl for RoundFunc {
};
match (value_scalar, args.return_type()) {
+ (value_scalar, return_type)
+ if is_integer_data_type(return_type) && dp >= 0 =>
+ {
+
ColumnarValue::Scalar(value_scalar.clone()).cast_to(return_type, None)
+ }
+ (value_scalar, Float64)
+ if is_integer_data_type(&value_scalar.data_type()) =>
+ {
+ let value = ColumnarValue::Scalar(value_scalar.clone())
+ .cast_to(&Float64, None)?;
+ match value {
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(v)))
=> {
+ let rounded = round_float(v, dp)?;
+
Ok(ColumnarValue::Scalar(ScalarValue::Float64(Some(rounded))))
+ }
+ ColumnarValue::Scalar(ScalarValue::Float64(None)) => {
+
Ok(ColumnarValue::Scalar(ScalarValue::Float64(None)))
+ }
+ _ => internal_err!(
+ "Unexpected datatype after casting integer
argument to Float64"
+ ),
+ }
+ }
Review Comment:
This `(value_scalar, Float64) if is_integer_data_type(value)` arm and
the`round_integer_array_to_float64!` macro + the eight `(IntN, Float64)` array
arms are unreachable:
##########
datafusion/functions/src/math/round.rs:
##########
@@ -245,6 +260,7 @@ impl ScalarUDFImpl for RoundFunc {
// extra precision to accommodate potential carry-over.
let return_type =
match input_type {
+ input_type if is_integer_data_type(input_type) =>
input_type.clone(),
Review Comment:
returns the integer type for *all* scales, but only non-negative scales are
handled downstream. `round(arrow_cast(125,'Int64'), -1)` now fails, it returned
`130.0` before this PR.
##########
datafusion/sqllogictest/test_files/scalar.slt:
##########
Review Comment:
tests only cover non-negative scalar scale. There's no coverage for negative
scale, column scale, or negative scale on a column
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
