-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove ObjectStore from FileScanConfig and ListingTableConfig #2668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
@@ -105,10 +99,12 @@ impl ListingTableConfig { | |||
} | |||
|
|||
/// Infer `ListingOptions` based on `table_path` suffix. | |||
pub async fn infer_options(self) -> Result<Self> { | |||
pub async fn infer_options(self, ctx: &SessionState) -> Result<Self> { | |||
let store = ctx.runtime_env.object_store(&self.table_path)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
let mock_store = | ||
TestObjectStore::new_arc(&files.iter().map(|f| (*f, 10)).collect::<Vec<_>>()); | ||
let ctx = SessionContext::new(); | ||
register_test_store(&ctx, &files.iter().map(|f| (*f, 10)).collect::<Vec<_>>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool
Which issue does this PR close?
Part of #2489
Rationale for this change
Manually plumbing the ObjectStore around introduces the potential for inconsistency, increases code complexity, and complicates plan serialization.
What changes are included in this PR?
Follows on from #2578 (comment) and removes the ObjectStore from FileScanConfig and ListingTableConfig, instead determining them as needed from the session context.
Are there any user-facing changes?
Yes, this changes the API of FileScanConfig and ListingTableConfig.
Does this PR break compatibility with Ballista?
Probably...