theirix commented on code in PR #19303:
URL: https://github.com/apache/datafusion/pull/19303#discussion_r2616194624
##########
datafusion/functions/src/math/power.rs:
##########
@@ -647,4 +671,239 @@ mod tests {
"Not yet implemented: Negative scale is not yet supported value:
-1"
);
}
+
+ #[test]
+ fn test_power_coerce_types() {
+ let power_func = PowerFunc::new();
+
+ // Int64 base with Int64 exponent -> both stay Int64
+ let result = power_func
+ .coerce_types(&[DataType::Int64, DataType::Int64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Int64, DataType::Int64]);
+
+ // Float64 base with Float64 exponent -> both stay Float64
+ let result = power_func
+ .coerce_types(&[DataType::Float64, DataType::Float64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Int64 base with Float64 exponent -> base coerced to Float64
+ // This is needed because integer power doesn't support
negative/fractional exponents
+ let result = power_func
+ .coerce_types(&[DataType::Int64, DataType::Float64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Int32 base with Float32 exponent -> both coerced to Float64
+ let result = power_func
+ .coerce_types(&[DataType::Int32, DataType::Float32])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Null base with Float64 exponent -> base coerced to Float64
+ let result = power_func
+ .coerce_types(&[DataType::Null, DataType::Float64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Null base with Int64 exponent -> base stays Int64
+ let result = power_func
+ .coerce_types(&[DataType::Null, DataType::Int64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Int64, DataType::Int64]);
+ }
+
+ /// Test for issue where pow(10, -2.0) was failing with overflow error
+ /// when integer base was used with negative float exponent
+ #[test]
+ fn test_power_int_base_negative_float_exp() {
Review Comment:
We can omit it if it tested by `SELECT pow(2, -0.5)` SLT
##########
datafusion/sqllogictest/test_files/math.slt:
##########
@@ -696,6 +696,39 @@ query error DataFusion error: Arrow error: Compute error:
Signed integer overflo
select lcm(2, 9223372036854775803);
+## pow/power
Review Comment:
I've placed a TODO
`# TODO: check backward compatibility for a case with base in64 and exponent
float64 since the power coercion is introduced`
in
[`decimal.slt:948`](https://github.com/apache/datafusion/blob/5a01e68643a198a1aaa7124524d7be5be7df24ec/datafusion/sqllogictest/test_files/decimal.slt#L948),
and they are testing `datafusion.sql_parser.parse_float_as_decimal = true`
behaviour.
Tests in `math.slt` test pure float behaviour for this specific issue.
Unfortunately, decimal behaviour is not triggered. Can we port a few of them to
the `decimal.slt` to test the new flag?
If you've covered this case, let's remove that TODO.
##########
datafusion/functions/src/math/power.rs:
##########
@@ -647,4 +671,239 @@ mod tests {
"Not yet implemented: Negative scale is not yet supported value:
-1"
);
}
+
+ #[test]
+ fn test_power_coerce_types() {
+ let power_func = PowerFunc::new();
+
+ // Int64 base with Int64 exponent -> both stay Int64
+ let result = power_func
+ .coerce_types(&[DataType::Int64, DataType::Int64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Int64, DataType::Int64]);
+
+ // Float64 base with Float64 exponent -> both stay Float64
+ let result = power_func
+ .coerce_types(&[DataType::Float64, DataType::Float64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Int64 base with Float64 exponent -> base coerced to Float64
+ // This is needed because integer power doesn't support
negative/fractional exponents
+ let result = power_func
+ .coerce_types(&[DataType::Int64, DataType::Float64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Int32 base with Float32 exponent -> both coerced to Float64
+ let result = power_func
+ .coerce_types(&[DataType::Int32, DataType::Float32])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Null base with Float64 exponent -> base coerced to Float64
+ let result = power_func
+ .coerce_types(&[DataType::Null, DataType::Float64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Float64, DataType::Float64]);
+
+ // Null base with Int64 exponent -> base stays Int64
+ let result = power_func
+ .coerce_types(&[DataType::Null, DataType::Int64])
+ .unwrap();
+ assert_eq!(result, vec![DataType::Int64, DataType::Int64]);
+ }
+
+ /// Test for issue where pow(10, -2.0) was failing with overflow error
+ /// when integer base was used with negative float exponent
+ #[test]
+ fn test_power_int_base_negative_float_exp() {
+ // After coercion, Int64 base with Float64 exponent becomes Float64,
Float64
+ let arg_fields = vec![
+ Field::new("a", DataType::Float64, true).into(),
+ Field::new("b", DataType::Float64, true).into(),
+ ];
+ let args = ScalarFunctionArgs {
+ args: vec![
+ ColumnarValue::Array(Arc::new(Float64Array::from(vec![10.0,
2.0, 4.0]))),
+ ColumnarValue::Array(Arc::new(Float64Array::from(vec![-2.0,
-1.0, 0.5]))),
+ ],
+ arg_fields,
+ number_rows: 3,
+ return_field: Field::new("f", DataType::Float64, true).into(),
+ config_options: Arc::new(ConfigOptions::default()),
+ };
+ let result = PowerFunc::new()
+ .invoke_with_args(args)
+ .expect("failed to initialize function power");
+
+ match result {
+ ColumnarValue::Array(arr) => {
+ let floats = as_float64_array(&arr)
+ .expect("failed to convert result to a Float64Array");
+ assert_eq!(floats.len(), 3);
+ assert!((floats.value(0) - 0.01).abs() < 1e-10); // 10^-2 =
0.01
+ assert!((floats.value(1) - 0.5).abs() < 1e-10); // 2^-1 = 0.5
+ assert!((floats.value(2) - 2.0).abs() < 1e-10); // 4^0.5 = 2.0
+ }
+ ColumnarValue::Scalar(_) => {
+ panic!("Expected an array value")
+ }
+ }
+ }
+
+ /// Test edge cases: zero exponent (anything^0 = 1), one base (1^anything
= 1),
+ /// zero base, and negative bases
+ #[test]
+ fn test_power_edge_cases_f64() {
Review Comment:
It could be better to convert it to SLT, too.
We tried to introduce unit tests to test different code paths (e.g., array
vs. scalar, decimal vs. int), but edge cases with data are usually tested
better via an SLT.
##########
datafusion/functions/src/math/power.rs:
##########
@@ -647,4 +671,239 @@ mod tests {
"Not yet implemented: Negative scale is not yet supported value:
-1"
);
}
+
+ #[test]
+ fn test_power_coerce_types() {
Review Comment:
I like this, very useful unit test
##########
datafusion/functions/src/math/power.rs:
##########
@@ -172,33 +172,57 @@ impl ScalarUDFImpl for PowerFunc {
fn coerce_types(&self, arg_types: &[DataType]) -> Result<Vec<DataType>> {
let [arg1, arg2] = take_function_args(self.name(), arg_types)?;
- fn coerced_type_base(name: &str, data_type: &DataType) ->
Result<DataType> {
+ fn coerced_type_exp(name: &str, data_type: &DataType) ->
Result<DataType> {
match data_type {
DataType::Null => Ok(DataType::Int64),
d if d.is_floating() => Ok(DataType::Float64),
d if d.is_integer() => Ok(DataType::Int64),
- d if is_decimal(d) => Ok(d.clone()),
+ d if is_decimal(d) => Ok(DataType::Float64),
other => {
exec_err!("Unsupported data type {other:?} for {}
function", name)
}
}
}
- fn coerced_type_exp(name: &str, data_type: &DataType) ->
Result<DataType> {
+ // Determine the exponent type first, as it affects base coercion
+ let exp_type = coerced_type_exp(self.name(), arg2)?;
+
+ // For base coercion: if exponent is Float64, we need Float64 base too
+ // (since integer power doesn't support negative/fractional exponents)
+ fn coerced_type_base(
+ name: &str,
+ data_type: &DataType,
+ exp_type: &DataType,
+ ) -> Result<DataType> {
match data_type {
- DataType::Null => Ok(DataType::Int64),
+ DataType::Null => {
Review Comment:
Can we group these two arms closer? Different comments are valuable, so I
would suggest keeping them.
--
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]
