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

Remove scan_csv methods from LogicalPlanBuilder #2537

Merged
merged 8 commits into from
May 16, 2022

Conversation

andygrove
Copy link
Member

@andygrove andygrove commented May 15, 2022

Which issue does this PR close?

Part of #2536

Rationale for this change

  • LogicalPlanBuilder should be in same crate as LogicalPlan
  • LogicalPlanBuilder should not need to know about data sources & object store but just TableSource

What changes are included in this PR?

There will be separate PRs to remove the other scan methods (parquet, json, avro).

Are there any user-facing changes?

Yes, this changes the LogicalPlanBuilder API

@andygrove andygrove added the api change Changes the API exposed to users of the crate label May 15, 2022
@andygrove andygrove self-assigned this May 15, 2022
@github-actions github-actions bot added ballista datafusion Changes in the datafusion crate labels May 15, 2022
@andygrove andygrove changed the title WIP Remove scan_csv methods from LogicalPlanBuilder Remove scan_csv methods from LogicalPlanBuilder May 15, 2022
@andygrove andygrove marked this pull request as ready for review May 15, 2022 19:11
@andygrove andygrove marked this pull request as draft May 15, 2022 19:17
@andygrove andygrove changed the title Remove scan_csv methods from LogicalPlanBuilder WIP: Remove scan_csv methods from LogicalPlanBuilder May 15, 2022
@andygrove andygrove changed the title WIP: Remove scan_csv methods from LogicalPlanBuilder Remove scan_csv methods from LogicalPlanBuilder May 16, 2022
@andygrove andygrove marked this pull request as ready for review May 16, 2022 16:58
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.

I went through the test changes carefully; Very nice @andygrove

@@ -1236,39 +1212,17 @@ mod roundtrip_tests {

#[tokio::test]
async fn roundtrip_analyze() -> Result<()> {
let schema = Schema::new(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

nice cleanups


roundtrip_test!(plan);

Ok(())
}

#[ignore] // see https://github.com/apache/arrow-datafusion/issues/2546
Copy link
Contributor

Choose a reason for hiding this comment

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

@andygrove
Copy link
Member Author

Thanks for the review @alamb ... I am close to being able to move LogicalPlanBuilder to the datafusion-expr crate now.

@andygrove andygrove merged commit 5223d6f into apache:master May 16, 2022
@andygrove andygrove deleted the plan-builder-remove-csv branch May 16, 2022 22:12
korowa pushed a commit to korowa/arrow-datafusion that referenced this pull request May 18, 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 datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants