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

Lower &mut SessionContext to &SessionContext in substrait consumer/producer #7836

Closed
amartins23 opened this issue Oct 16, 2023 · 5 comments · Fixed by #7965
Closed

Lower &mut SessionContext to &SessionContext in substrait consumer/producer #7836

amartins23 opened this issue Oct 16, 2023 · 5 comments · Fixed by #7965
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@amartins23
Copy link

Is your feature request related to a problem or challenge?

Currently functions to convert from substrait to DataFusion plans, or to produce a substrait plan from a DataFusion logical plan, require a mutable reference to SessionContext. However, none of the implementations seems to require that. Since state access in the session context does not require a mutable pointer, I don't see the need to take a mutable reference to the session context.

Allowing shared references to SessionContext to be used would simplify consuming code and should not be a breaking as you can pass an &mut reference to a function expecting a shared reference.

Describe the solution you'd like

Change the various conversion functions in the datafusion-substrait crate to receive &SessionContext instead of &mut SessionContext.

Describe alternatives you've considered

No response

Additional context

No response

@amartins23 amartins23 added the enhancement New feature or request label Oct 16, 2023
@alamb
Copy link
Contributor

alamb commented Oct 18, 2023

Hi @amartins23

It seems like the consumer functions such as
https://docs.rs/datafusion-substrait/latest/datafusion_substrait/logical_plan/consumer/fn.from_substrait_plan.html
https://docs.rs/datafusion-substrait/latest/datafusion_substrait/physical_plan/consumer/fn.from_substrait_rel.html

Require mutable access, but the producers do not (they already take &SessionContext, not &mut SessionContext)
https://docs.rs/datafusion-substrait/latest/datafusion_substrait/logical_plan/producer/fn.to_substrait_plan.html

This I think this ticket would require:

  1. Converting from &mut SessionContext to &SessionContext
  2. Ideally add some doc comments / examples of how to do this conversion

This seems straightforward and a good first project so marking it as such

Note there may be some subtlety I don't understand that would make this not feasible.

@alamb alamb added the good first issue Good for newcomers label Oct 18, 2023
@my-vegetable-has-exploded
Copy link
Contributor

I'd like to have a try.

@my-vegetable-has-exploded
Copy link
Contributor

2. Ideally add some doc comments / examples of how to do this conversion

Sorry for the delay in completing this task. Would an example like this be OK?

@alamb
Copy link
Contributor

alamb commented Oct 29, 2023

Sorry for the delay in completing this task.

No worries -- thank you for doing so!

@alamb
Copy link
Contributor

alamb commented Oct 29, 2023

Fixed in #7965

@alamb alamb closed this as completed Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
3 participants