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

add cross rt execution code #13634

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

add cross rt execution code #13634

wants to merge 1 commit into from

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Dec 3, 2024

#12393 (comment)

I'm not sure where to put this or what the right API is, but I'm putting this up as a first draft for discussion.

@adriangb
Copy link
Contributor Author

adriangb commented Dec 3, 2024

I suspect the right way to implement this would be to make SessionContext a trait and offer this as an implementation of it? There might even be a way to automatically wrap all ObjectStores in CrossRtObjectStore. The only manual bit left then would be to ensure any non-ObjectStore IO is wrapped in spawn_io, that's made easier by the fact that we don't allow IO on the CPU runtime so you'd know if you forgot (you get a panic).

@tustvold
Copy link
Contributor

tustvold commented Dec 4, 2024

See also #13424 which contains some prior discussion, including why shimming ObjectStore in this way is potentially fraught (#13424 (comment) in particular)

@adriangb
Copy link
Contributor Author

adriangb commented Dec 4, 2024

Interesting, thanks! We don't use list (only get) so maybe that's why we haven't run into any issues. It sounds like AsyncFileReader is a better place for the shim. I wonder if it could be put in there in such a way that it falls back to running on the current runtime, such that we don't need a separate implementation?

@alamb
Copy link
Contributor

alamb commented Dec 8, 2024

See also #13424 which contains some prior discussion, including why shimming ObjectStore in this way is potentially fraught (#13424 (comment) in particular)

FWIW I tried what I think is the alternate suggestion in #13690 and can't seem to make it work realistically: #13690 (comment)

So TLDR unless someone can come up with anything else, the approach in this PR is the only one that seems practical to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants