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

datafusion_proto crate should have exhaustive match statements for handling Expr #2565

Closed
andygrove opened this issue May 17, 2022 · 4 comments · Fixed by #2567
Closed

datafusion_proto crate should have exhaustive match statements for handling Expr #2565

andygrove opened this issue May 17, 2022 · 4 comments · Fixed by #2567
Labels
enhancement New feature or request

Comments

@andygrove
Copy link
Member

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
When adding new Expr variants, we should also be adding code to datafusion_proto to handle them but we are not forced into doing this today because the match statement handles the _ case with an error.

Describe the solution you'd like
Support all current Expr variants in and remove the _ case.

Describe alternatives you've considered
None

Additional context
None

@andygrove andygrove added enhancement New feature or request good first issue Good for newcomers and removed good first issue Good for newcomers labels May 17, 2022
@andygrove
Copy link
Member Author

Currently datafusion.proto only handles expressions and not logical plans, but that will have to change if we want to support subquery expressions. Ballista already has support for plans so maybe we move some of that code to DataFusion? Another option would be to look at Substrait but that may be a significant amount of work.

fyi @alamb @carols10cents @Ted-Jiang @thinkharderdev since you have contributed to this file

@alamb
Copy link
Contributor

alamb commented May 18, 2022

I would think we should move the code for serializing logical plans into datafusion.proto as well

@thinkharderdev
Copy link
Contributor

Yeah, agreed. I think it makes sense to move all serialization into datafusion-proto. It might be useful as well if we move forward with breaking Ballista out into it's own repo to help ensure that constructs added to DataFusion are always serializable.

@Ted-Jiang
Copy link
Member

+1 , remove the _ case, will give error at compile time.

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.

4 participants