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

LogicalPlanBuilder now uses TableSource instead of TableProvider #2569

Merged
merged 6 commits into from
May 21, 2022

Conversation

andygrove
Copy link
Member

@andygrove andygrove commented May 18, 2022

Which issue does this PR close?

Closes #2346

Rationale for this change

SQL planner should use TableSource not TableProvider since there is no need to actually perform scans during planning and this would be a step towards moving the SQL planner into its own crate so we have cleaner dependency management.

What changes are included in this PR?

Use TableSource not TableProvider.

Are there any user-facing changes?

Yes, API change.

@andygrove andygrove added the api change Changes the API exposed to users of the crate label May 18, 2022
@andygrove andygrove self-assigned this May 18, 2022
@github-actions github-actions bot added ballista datafusion Changes in the datafusion crate labels May 18, 2022
@@ -17,7 +17,6 @@

//! This module provides a builder for creating LogicalPlans

use crate::datasource::TableProvider;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the main objective - removing one of the final dependencies from LogicalPlanBuilder to the "core" datafusion crate.

@andygrove andygrove changed the title WIP: LogicalPlanBuilder now uses TableSource instead of TableProvider LogicalPlanBuilder now uses TableSource instead of TableProvider May 18, 2022
@andygrove andygrove marked this pull request as ready for review May 18, 2022 21:31
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.

Very nice @andygrove -- thank you!

}

/// Convert a table provider into a builder with a TableScan
pub fn scan_with_filters(
table_name: impl Into<String>,
provider: Arc<dyn TableProvider>,
table_source: Arc<dyn TableSource>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could make the APIs a little (avoid having to call provider_as_source) nicer with something like

Untested:

table_source: impl Into<Arc<dyn TableSource>>

Or add some other trait that would allow us to pass both Arc<dyn TableSource> as well as Arc<dyn TableProvider>

I think we can always refine the API in a follow on PR

@alamb alamb merged commit cb84504 into apache:master May 21, 2022
@andygrove andygrove deleted the lpb-remove-table-provider branch January 27, 2023 18:57
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 datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL planner should use TableSource not TableProvider
2 participants