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

Pass SessionState to TableProvider::scan #2660

Merged
merged 3 commits into from
May 31, 2022

Conversation

tustvold
Copy link
Contributor

Which issue does this PR close?

Closes #2658
Closes #2657

Rationale for this change

Prevents circular references, and will also allow TableProviders access to things like the RuntimeEnv, etc...

What changes are included in this PR?

Passes a &SessionState to TableProvider::scan, this is necessary for TableProvider to be able to access plan state without potentially introducing circular references.

Are there any user-facing changes?

Yes, this changes the signature of TableProvider::scan

Does this PR break compatibility with Ballista?

Possibly

@github-actions github-actions bot added core Core DataFusion crate datafusion Changes in the datafusion crate labels May 30, 2022
@tustvold tustvold force-pushed the session-state-table-provider branch from b757bb7 to 3290e26 Compare May 30, 2022 19:24
@@ -931,6 +931,11 @@ impl SessionContext {
pub fn task_ctx(&self) -> Arc<TaskContext> {
Arc::new(TaskContext::from(self))
}

/// Get a copy of the [`SessionState`] of this [`SessionContext`]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a massive fan of this, but it appears to be what we do already for every query and when constructing a TaskContext, so it can't be that bad..........

@tustvold tustvold force-pushed the session-state-table-provider branch from 3290e26 to 4ae3b42 Compare May 30, 2022 19:27
@tustvold
Copy link
Contributor Author

Will fix ballista after apache/datafusion-ballista#48 and apache/datafusion-ballista#49

@github-actions github-actions bot added the development-process Related to development process of DataFusion label May 31, 2022
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @andygrove as I believe he is working on disconnecting the logical and physical plans in DataFusion and I want to make sure this doesn't mess up his plans

Otherwise looks good to me though

@tustvold
Copy link
Contributor Author

My understanding is that #2569 removed the dependency of LogicalPlan on TableProvider

@tustvold tustvold merged commit 8f514f9 into apache:master May 31, 2022
@tustvold tustvold added the api change Changes the API exposed to users of the crate label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate datafusion Changes in the datafusion crate development-process Related to development process of DataFusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass SessionState to TableProvider ViewTable Circular Reference
3 participants