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

Move LogicalPlan to datafusion-expr crate #2245

Closed
Tracked by #474 ...
andygrove opened this issue Apr 15, 2022 · 5 comments · Fixed by #2294
Closed
Tracked by #474 ...

Move LogicalPlan to datafusion-expr crate #2245

andygrove opened this issue Apr 15, 2022 · 5 comments · Fixed by #2294
Labels
enhancement New feature or request

Comments

@andygrove
Copy link
Member

andygrove commented Apr 15, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
I am trying to implement support for the expression EXISTS (<subquery>) and this requires having a new Expr variant that refers to a LogicalPlan. However, this is not possible because LogicalPlan is in the core datafusion crate which depends on the datafusion-expr crate that contains Expr, and therefore it is not possible to reference LogicalPlan from Expr.

Describe the solution you'd like
LogicalPlan and Expr should live in the same crate IMO so I propose moving LogicalPlan into the datafusion-expr crate.

Describe alternatives you've considered
None

Additional context
See the discussion in #2181 for more context

@matthewmturner
Copy link
Contributor

Would it make sense to call this crate 'datafusion-logical-plan' after this change? At least to me it wouldn't be immediately obvious that logical plan was there from naming. But maybe that's just me.

@andygrove
Copy link
Member Author

Would it make sense to call this crate 'datafusion-logical-plan' after this change? At least to me it wouldn't be immediately obvious that logical plan was there from naming. But maybe that's just me.

Yes, that would make more sense to me too.

@andygrove
Copy link
Member Author

andygrove commented Apr 16, 2022

So this doesn't look like it can work. The logical plan depends on the datasource module, which depends on the physical plan 😞

It might be better just to move the expressions back into the core crate.

@andygrove andygrove reopened this Apr 16, 2022
@andygrove
Copy link
Member Author

We need to address #2247 before we can make progress on this issue

@andygrove andygrove mentioned this issue Apr 16, 2022
15 tasks
@jychen7
Copy link
Contributor

jychen7 commented Apr 16, 2022

Would it make sense to call this crate 'datafusion-logical-plan' after this change? At least to me it wouldn't be immediately obvious that logical plan was there from naming. But maybe that's just me.

+1
[minor] given we also have physical-expr, maybe we call it logical-expr?

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
3 participants