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.

Sign up for GitHub

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

Jump to bottom

refactor: Make catalog datasource #14643

Merged
merged 5 commits into from
Feb 14, 2025
Merged

refactor: Make catalog datasource #14643

merged 5 commits into from
Feb 14, 2025

Conversation

logan-keede
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

refer to discussion in #14616

What changes are included in this PR?

making datafusion-datasource

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Feb 13, 2025
@logan-keede
Copy link
Contributor Author

do we want DataSource trait in datasource crate? will that be a breaking change since we won't be able to re-export DataSource in datafusion_physical_plan without non-trivial refactoring or is it okay to just keep a duplicate code till we release Datafusion 46.0.0?

@logan-keede
Copy link
Contributor Author

do we want DataSource trait in datasource crate? will that be a breaking change since we won't be able to re-export DataSource in datafusion_physical_plan without non-trivial refactoring or is it okay to just keep a duplicate code till we release Datafusion 46.0.0?

cc @alamb @jayzhan211

@jayzhan211
Copy link
Contributor

jayzhan211 commented Feb 13, 2025 •
edited
Loading

We should! It is in physical-plan because there is no datasource then. It should be part of datasource crate.

@alamb
Copy link
Contributor

alamb commented Feb 13, 2025

do we want DataSource trait in datasource crate? will that be a breaking change since we won't be able to re-export DataSource in datafusion_physical_plan without non-trivial refactoring or is it okay to just keep a duplicate code till we release Datafusion 46.0.0?

To be clear too, I think DataSource was added since we released DataFusion 45

https://docs.rs/datafusion/latest/datafusion/index.html?search=DataSource

Thus I do't think moving DataSource to datasource is a breaking API change

alamb
alamb approved these changes Feb 13, 2025
View reviewed changes
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 love it -- this is great. Thank you @logan-keede

Also thank you for the datasource suggestion @jayzhan211 -- 💯

datafusion/catalog-listing/Cargo.toml
datafusion-catalog = { workspace = true }
datafusion-common = { workspace = true, features = ["object_store"] }
datafusion-common-runtime = { workspace = true }
datafusion-datasource = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

datafusion/datasource/README.md Outdated Show resolved Hide resolved
@logan-keede
Copy link
Contributor Author

Thanks for the feedback!! @alamb @jayzhan211

To be clear too, I think DataSource was added since we released DataFusion 45

😅 totally overlooked that

@alamb alamb merged commit e11a8ca into apache:main Feb 14, 2025
26 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 14, 2025

Thanks for the feedback!! @alamb @jayzhan211

To be clear too, I think DataSource was added since we released DataFusion 45

😅 totally overlooked that

Things are moving fast 🏃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@logan-keede @jayzhan211 @alamb