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

apply constant folding to LogicalPlan::Values #1170

Closed
jimexist opened this issue Oct 23, 2021 · 3 comments · Fixed by #1355
Closed

apply constant folding to LogicalPlan::Values #1170

jimexist opened this issue Oct 23, 2021 · 3 comments · Fixed by #1355

Comments

@jimexist
Copy link
Member

I suspect it is not likely to matter, but constant folding could be applied to the Exprs in values. As written this code will not apply constant folding to those expressions

Originally posted by @alamb in #1165 (comment)

@alamb alamb changed the title apply constant folding to values logical plan apply constant folding to LogivalPlan::Values Oct 25, 2021
@alamb alamb changed the title apply constant folding to LogivalPlan::Values apply constant folding to LogicalPlan::Values Oct 25, 2021
@viirya
Copy link
Member

viirya commented Nov 24, 2021

Hmm, I actually saw LogicalPlan::Values among the logical plans in constant folding optimizer rule now. Is this already resolved?

@viirya
Copy link
Member

viirya commented Nov 24, 2021

Ran a test:

    #[test]
    fn optimize_plan_support_values() -> Result<()> {
        let expr1 = Expr::BinaryExpr {
            left: Box::new(lit(1)),
            op: Operator::Plus,
            right: Box::new(lit(2)),
        };
        let expr2 = Expr::BinaryExpr {
            left: Box::new(lit(2)),
            op: Operator::Minus,
            right: Box::new(lit(1)),
        };
        let values = vec![vec![expr1, expr2]];
        let plan = LogicalPlanBuilder::values(values)?.build()?;

        let expected = "\
        Values: (Int32(3), Int32(1))";

        assert_optimized_plan_eq(&plan, expected);
        Ok(())
    }

Looks it already did constant folding?

Maybe we just need to add the test?

@alamb
Copy link
Contributor

alamb commented Nov 24, 2021

Thanks @viirya -- yes I think the general purpose constant folding implementation we have now should handle the Values. Thanks for the test !

@alamb alamb changed the title apply constant folding to LogicalPlan::Values apply constant folding to LogicalPlan::Values Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants