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

Make the conversion from logical to physical plan async #1012

Closed
rdettai opened this issue Sep 17, 2021 · 2 comments · Fixed by #1013
Closed

Make the conversion from logical to physical plan async #1012

rdettai opened this issue Sep 17, 2021 · 2 comments · Fixed by #1013
Labels
enhancement New feature or request

Comments

@rdettai
Copy link
Contributor

rdettai commented Sep 17, 2021

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
As discussed in #1010, it would add a lot of flexibility to the planning to make it possible to use async methods. Thanks to #962, the statistics resolution can be delayed to the creation of the physical plan. Adding async to TableProvider.scan(), the method that converts the logical plan to the physical plan, will make it possible to have async statistics resolution.

Describe the solution you'd like
Make the trait method TableProvider.scan() async and propagate that change to the outer APIs (mostly ExecutionContext.create_physical_plan(&LogicalPlan))

Describe alternatives you've considered

  • also add statistics to optimizers: that could be a next step but it is not required for now
  • leave the planning sync and force TableProvider implementers to make their async methods sync: syncifying in Rust is pretty hard (cloud-reader-rs is an example of this) and it is a huge constraint for implementers

Additional context
This will also help the useage of the ObjectStore abstraction added in #811.

@rdettai
Copy link
Contributor Author

rdettai commented Sep 17, 2021

This also pops a question. The state of ExecutionContext is protected by a Mutex:

pub struct ExecutionContext {
    /// Internal state for the context
    pub state: Arc<Mutex<ExecutionContextState>>,
}

Why do some of its methods take &mut self ???

@houqp
Copy link
Member

houqp commented Sep 20, 2021

Why do some of its methods take &mut self ???

Probably for no good reason, maybe it used to not be protected by mutex.

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