Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LogFunc simplifier swaps the order of arguments #10359

Closed
erratic-pattern opened this issue May 2, 2024 · 0 comments · Fixed by #10360
Closed

LogFunc simplifier swaps the order of arguments #10359

erratic-pattern opened this issue May 2, 2024 · 0 comments · Fixed by #10360
Labels
bug Something isn't working

Comments

@erratic-pattern
Copy link
Contributor

erratic-pattern commented May 2, 2024

Describe the bug

The LogFunc::simplify function swaps the order of arguments.

To Reproduce

I have created a test case that can reproduce the issue:

    #[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();
        match result {
            ExprSimplifyResult::Simplified(_) => panic!("Expected ExprSimplifyResult::Original"),
            ExprSimplifyResult::Original(args) => {
                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();
        match result {
            ExprSimplifyResult::Simplified(_) => panic!("Expected ExprSimplifyResult::Original"),
            ExprSimplifyResult::Original(args) => {
                assert_eq!(args.len(), 2);
                assert_eq!(args[0], lit(2));
                assert_eq!(args[1], lit(3));
            }
        }
    }

test results:

---- math::log::tests::test_log_simplify_original stdout ----
thread 'math::log::tests::test_log_simplify_original' panicked at datafusion/functions/src/math/log.rs:330:17:
assertion `left == right` failed
  left: Literal(Int32(3))
 right: Literal(Int32(2))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Expected behavior

Arguments should remain in correct order of log(base, number)

Additional context

This bug seems to be hidden by the fact that the expression simplifier runs twice. In most cases this means the arguments will swap back into place, but it may be possible to craft expressions which only run simplify once.

While working on PR #10358 to run the simplifer in a loop, I began to encounter this bug in the case of 3 iterations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant