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

Remove duplicate tests from test_const_evaluator_scalar_functions #1727

Closed
HaoYang670 opened this issue Feb 2, 2022 · 5 comments · Fixed by #2085 or #2089
Closed

Remove duplicate tests from test_const_evaluator_scalar_functions #1727

HaoYang670 opened this issue Feb 2, 2022 · 5 comments · Fixed by #2085 or #2089

Comments

@HaoYang670
Copy link
Contributor

Describe the bug
I find that we have 2 same tests:

        // check that non foldable arguments are folded
        // to_timestamp(a) --> to_timestamp(a) [no rewrite possible]
        let expr = Expr::ScalarFunction {
            args: vec![col("a")],
            fun: BuiltinScalarFunction::ToTimestamp,
        };
        test_evaluate(expr.clone(), expr);


        // check that non foldable arguments are folded
        // to_timestamp(a) --> to_timestamp(a) [no rewrite possible]
        let expr = Expr::ScalarFunction {
            args: vec![col("a")],
            fun: BuiltinScalarFunction::ToTimestamp,
        };
        test_evaluate(expr.clone(), expr);

https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/optimizer/simplify_expressions.rs#L1026-L1040

To Reproduce

Expected behavior
Remove one of the test or add some comment about why we need 2 same tests.

Additional context

@HaoYang670 HaoYang670 added the bug Something isn't working label Feb 2, 2022
@HaoYang670
Copy link
Contributor Author

Another that I find
https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/optimizer/simplify_expressions.rs#L1042-L1054

        // volatile / stable functions should not be evaluated
        // rand() + (1 + 2) --> rand() + 3
        let fun = BuiltinScalarFunction::Random;
        assert_eq!(fun.volatility(), Volatility::Volatile);
        let rand = Expr::ScalarFunction { args: vec![], fun };
        let expr = rand.clone() + (lit(1) + lit(2));
        let expected = rand + lit(3);
        test_evaluate(expr, expected);


        // parenthesization matters: can't rewrite
        // (rand() + 1) + 2 --> (rand() + 1) + 2)
        let fun = BuiltinScalarFunction::Random;
        assert_eq!(fun.volatility(), Volatility::Volatile);

We test BuiltinScalarFunction::Random.volatility() twice.

@HaoYang670 HaoYang670 changed the title Remove duplicate tests in test_const_evaluator_scalar_functions Remove duplicate tests from test_const_evaluator_scalar_functions Feb 3, 2022
@bridyash13
Copy link

Hey @HaoYang670 can I work on this issue?

@HaoYang670
Copy link
Contributor Author

Sure. Thank you, @bridyash13

@jackwener
Copy link
Member

jackwener commented Mar 25, 2022

I found the problem to be a very simple one, and it didn't seem to be progressing. so I fix it.

BTW, I search the bug label and find it, and it shouldn't be a bug but an enhancement.

jackwener added a commit to jackwener/arrow-datafusion that referenced this issue Mar 25, 2022
xudong963 pushed a commit that referenced this issue Mar 25, 2022
@xudong963 xudong963 reopened this Mar 25, 2022
@xudong963 xudong963 removed the bug Something isn't working label Mar 25, 2022
@jackwener
Copy link
Member

I make a stupid mistake....ignored the supplement following.😅 I will add another PR......

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants