kosiew commented on code in PR #24399:
URL: https://github.com/apache/datafusion/pull/24399#discussion_r3802431909
##########
datafusion/functions/src/math/log.rs:
##########
Review Comment:
I think this simplification can bypass the new invariant. `log(a, a)` is
simplified directly to `1`, so the logged value is never evaluated. For
example, `SELECT log(0.0::float8, 0.0::float8)` returns `1.0` on this commit
instead of the expected domain error.
The similar `log(a, power(a, b))` rewrite around lines 405-412 can also
bypass the check when the power result is zero.
Could we guard these rewrites so they only apply when the domain-error
preconditions are known to hold? It would also be good to add planner-level
regression cases for these zero-valued forms.
##########
datafusion/functions/src/math/log.rs:
##########
@@ -1213,4 +1243,135 @@ mod tests {
}
}
}
+
+ fn invoke_log(
+ args: Vec<ColumnarValue>,
+ data_types: Vec<DataType>,
+ ) -> Result<ColumnarValue> {
+ let number_rows = args
+ .iter()
+ .map(|a| match a {
+ ColumnarValue::Array(arr) => arr.len(),
+ ColumnarValue::Scalar(_) => 1,
+ })
+ .max()
+ .unwrap_or(1);
+ let arg_fields = data_types
+ .into_iter()
+ .map(|dt| Field::new("a", dt, false).into())
+ .collect();
+ let args = ScalarFunctionArgs {
+ args,
+ arg_fields,
+ number_rows,
+ return_field: Field::new("f", DataType::Float64, true).into(),
+ config_options: Arc::new(ConfigOptions::default()),
+ };
+ LogFunc::new().invoke_with_args(args)
+ }
+
+ fn assert_log_of_zero(err: datafusion_common::DataFusionError) {
+ let message = err.to_string();
+ assert!(
+ message.contains(LOG_OF_ZERO_ERROR),
+ "expected '{LOG_OF_ZERO_ERROR}' in error, got {message}"
+ );
+ }
+
+ #[test]
+ fn test_log_zero_float64_unary_errors() {
+ let err = invoke_log(
+ vec![ColumnarValue::Scalar(ScalarValue::Float64(Some(0.0)))],
+ vec![DataType::Float64],
+ )
+ .expect_err("log(0.0) should be a domain error");
+ assert_log_of_zero(err);
+ }
+
+ #[test]
+ fn test_log_negative_zero_float64_errors() {
+ let err = invoke_log(
+ vec![ColumnarValue::Scalar(ScalarValue::Float64(Some(-0.0)))],
+ vec![DataType::Float64],
+ )
+ .expect_err("log(-0.0) should be a domain error");
+ assert_log_of_zero(err);
+ }
+
+ #[test]
+ fn test_log_zero_float32_unary_errors() {
+ let err = invoke_log(
+ vec![ColumnarValue::Scalar(ScalarValue::Float32(Some(0.0)))],
+ vec![DataType::Float32],
+ )
+ .expect_err("log(0.0f32) should be a domain error");
+ assert_log_of_zero(err);
+ }
+
+ #[test]
+ fn test_log_zero_float64_binary_errors() {
+ let err = invoke_log(
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(10.0))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(0.0))),
+ ],
+ vec![DataType::Float64, DataType::Float64],
+ )
+ .expect_err("log(10, 0.0) should be a domain error");
+ assert_log_of_zero(err);
+ }
+
+ #[test]
+ fn test_log_zero_array_errors() {
+ let err = invoke_log(
+ vec![ColumnarValue::Array(Arc::new(Float64Array::from(vec![
+ 10.0, 0.0, 100.0,
+ ])))],
+ vec![DataType::Float64],
+ )
+ .expect_err("log() of an array containing 0 should be a domain error");
+ assert_log_of_zero(err);
+ }
+
+ #[test]
+ fn test_log_zero_decimal128_errors() {
+ let err = invoke_log(
Review Comment:
Nice to see the new coverage for Float32/64 and Decimal128/256. Since this
change also touches the separate Float16, Decimal32, and Decimal64 branches,
could we add a small table-driven unit test that exercises every changed
physical type?
That should help catch branch-specific regressions 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
