Skip to content

Commit

Permalink
fix: LogFunc simplify swaps arguments (#10360)
Browse files Browse the repository at this point in the history
* fix: LogFunc simplify swaps arguments

* refactor tests with let else
  • Loading branch information
erratic-pattern authored May 3, 2024
1 parent 3b245ff commit b21bf9e
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 2 deletions.
1 change: 1 addition & 0 deletions datafusion/expr/src/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
50 changes: 48 additions & 2 deletions datafusion/functions/src/math/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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::*;

Expand Down Expand Up @@ -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));
}
}

0 comments on commit b21bf9e

Please sign in to comment.