This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new b21bf9e852 fix: LogFunc simplify swaps arguments (#10360)
b21bf9e852 is described below
commit b21bf9e8527de82e901c3a61127d63779f230163
Author: Adam Curtis <[email protected]>
AuthorDate: Thu May 2 20:54:54 2024 -0400
fix: LogFunc simplify swaps arguments (#10360)
* fix: LogFunc simplify swaps arguments
* refactor tests with let else
---
datafusion/expr/src/simplify.rs | 1 +
datafusion/functions/src/math/log.rs | 50 ++++++++++++++++++++++++++++++++++--
2 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/datafusion/expr/src/simplify.rs b/datafusion/expr/src/simplify.rs
index 6fae31b4a6..ccf45ff0d0 100644
--- a/datafusion/expr/src/simplify.rs
+++ b/datafusion/expr/src/simplify.rs
@@ -109,6 +109,7 @@ impl<'a> SimplifyInfo for SimplifyContext<'a> {
}
/// Was the expression simplified?
+#[derive(Debug)]
pub enum ExprSimplifyResult {
/// The function call was simplified to an entirely new Expr
Simplified(Expr),
diff --git a/datafusion/functions/src/math/log.rs
b/datafusion/functions/src/math/log.rs
index b828739126..f451321ea1 100644
--- a/datafusion/functions/src/math/log.rs
+++ b/datafusion/functions/src/math/log.rs
@@ -192,7 +192,7 @@ impl ScalarUDFImpl for LogFunc {
} else {
let args = match num_args {
1 => vec![number],
- 2 => vec![number, base],
+ 2 => vec![base, number],
_ => {
return internal_err!(
"Unexpected number of arguments in
log::simplify"
@@ -220,7 +220,13 @@ fn is_pow(func_def: &ScalarFunctionDefinition) -> bool {
#[cfg(test)]
mod tests {
- use datafusion_common::cast::{as_float32_array, as_float64_array};
+ use std::collections::HashMap;
+
+ use datafusion_common::{
+ cast::{as_float32_array, as_float64_array},
+ DFSchema,
+ };
+ use datafusion_expr::{execution_props::ExecutionProps,
simplify::SimplifyContext};
use super::*;
@@ -283,4 +289,44 @@ mod tests {
}
}
}
+ #[test]
+ // Test log() simplification errors
+ fn test_log_simplify_errors() {
+ let props = ExecutionProps::new();
+ let schema =
+ Arc::new(DFSchema::new_with_metadata(vec![],
HashMap::new()).unwrap());
+ let context = SimplifyContext::new(&props).with_schema(schema);
+ // Expect 0 args to error
+ let _ = LogFunc::new().simplify(vec![], &context).unwrap_err();
+ // Expect 3 args to error
+ let _ = LogFunc::new()
+ .simplify(vec![lit(1), lit(2), lit(3)], &context)
+ .unwrap_err();
+ }
+
+ #[test]
+ // Test that non-simplifiable log() expressions are unchanged after
simplification
+ fn test_log_simplify_original() {
+ let props = ExecutionProps::new();
+ let schema =
+ Arc::new(DFSchema::new_with_metadata(vec![],
HashMap::new()).unwrap());
+ let context = SimplifyContext::new(&props).with_schema(schema);
+ // One argument with no simplifications
+ let result = LogFunc::new().simplify(vec![lit(2)], &context).unwrap();
+ let ExprSimplifyResult::Original(args) = result else {
+ panic!("Expected ExprSimplifyResult::Original")
+ };
+ assert_eq!(args.len(), 1);
+ assert_eq!(args[0], lit(2));
+ // Two arguments with no simplifications
+ let result = LogFunc::new()
+ .simplify(vec![lit(2), lit(3)], &context)
+ .unwrap();
+ let ExprSimplifyResult::Original(args) = result else {
+ panic!("Expected ExprSimplifyResult::Original")
+ };
+ assert_eq!(args.len(), 2);
+ assert_eq!(args[0], lit(2));
+ assert_eq!(args[1], lit(3));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
