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 ExecutionPlan Sync #2307

Closed
tustvold opened this issue Apr 21, 2022 · 0 comments · Fixed by #2434
Closed

Make ExecutionPlan Sync #2307

tustvold opened this issue Apr 21, 2022 · 0 comments · Fixed by #2434
Labels
enhancement New feature or request

Comments

@tustvold
Copy link
Contributor

tustvold commented Apr 21, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

The fact ExecutionPlan::execute is async seems to be a very common source of bugs:

On a holistic level I can't see why it needs to be async, given it returns an async stream which can/should contain any deferred computations, as opposed to doing them synchronously with the plan's construction.

Describe the solution you'd like

I would like to make ExecutionPlan::execute sync, which will in turn make ExecutionPlan itself sync

i.e. change the signature from

async fn execute(
        &self,
        partition: usize,
        context: Arc<TaskContext>,
    ) -> Result<SendableRecordBatchStream>;

To

fn execute(
        &self,
        partition: usize,
        context: Arc<TaskContext>,
    ) -> Result<SendableRecordBatchStream>

Describe alternatives you've considered

We could not do this

Additional context

The new scheduler has a slightly ugly hack to workaround this - https://github.com/apache/arrow-datafusion/pull/2226/files#diff-b3e57d18925cb116e868da5f997735bdf6f937d53635e85bc227c4e01a2e2b33R138

This will likely help with future work to de-asyncify the physical plan - see #2199

It may also help with compile times, and error messages, both downsides of using async_trait

@tustvold tustvold added the enhancement New feature or request label Apr 21, 2022
@tustvold tustvold changed the title Make ExecutionPlan::execute Sync Make ExecutionPlan Sync Apr 21, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue May 3, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue May 4, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue May 4, 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.

1 participant