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

Logical optimizer rule "simplify expressions" should not depend on the core datafusion crate #2535

Closed
Tracked by #474
andygrove opened this issue May 15, 2022 · 2 comments · Fixed by #2682
Closed
Tracked by #474
Assignees
Labels
enhancement New feature or request

Comments

@andygrove
Copy link
Member

andygrove commented May 15, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
In the "simplify expressions" rule, we evaluate and fold expressions that result in scalar values and we depend on the physical expression crate to do this, which is fine, but we also depend on the core crate for create_physical_expr and we can't have the new datafusion-optimizer crate depend on core.

Describe the solution you'd like
It looks like we should be able to move create_physical_expr and associated functions to the datafusion-physical-expr crate.

Describe alternatives you've considered
None

Additional context
This is related to #2345

@andygrove andygrove added the enhancement New feature or request label May 15, 2022
@andygrove andygrove changed the title Logical optimizer rule "simplify expressions" should not depend on physical expressions Logical optimizer rule "simplify expressions" should not depend on physical planner and expressions May 23, 2022
@alamb
Copy link
Contributor

alamb commented May 24, 2022

I think it is important for expression simplification that expressions can be partially evaluated at optimization time.

In terms of evaluating on logical plan Exprs directly, I agree there is no theoretical reason this can not be done, but if we did so it will result in dual expression evaluation systems that would have be kept in sync. This might be especially hard if someone wanted to use a different physical expression evaluation system

I understand the desire to remove the direct code dependence on the execution engine from the optimizer -- perhaps we can do so via a trait -- for example, we could pass some sort of dyn ExprEvaluator to the optimizer for use in simplify expressions and elsewhere.

By default, DataFusion could provide the evaluator based on PhysicalExpr -- and for those users of the optimizer itself they could decide the most appropriate way to provide expression evaluation to the optimizer

@andygrove
Copy link
Member Author

@alamb Now that I have started looking at this more closely it turns out to be less of an issue than I first thought. We just need to move create_physical_expr from core to the physical-expr crate. I have updated the issue description.

@andygrove andygrove self-assigned this Jun 2, 2022
@andygrove andygrove changed the title Logical optimizer rule "simplify expressions" should not depend on physical planner and expressions Logical optimizer rule "simplify expressions" should not depend on the core datafusion crate Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants