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

make datafusion-catalog-listing and move some implementation of listing out of datafusion/core/datasource/listing #14464

Merged
merged 12 commits into from
Feb 5, 2025

Conversation

logan-keede
Copy link
Contributor

@logan-keede logan-keede commented Feb 3, 2025

Which issue does this PR close?

Rationale for this change

Decrease build time, Increase Modularity.

What changes are included in this PR?

Moving some part of datasource/listing to datafusion-catalog-listing

Are these changes tested?

As much as I could on my PC by CI.

Are there any user-facing changes?

There should not be any user-facing changes, it is only a refactor.

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

cc @alamb

@alamb
Copy link
Contributor

alamb commented Feb 4, 2025

cc @alamb

😍

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.

Thanks @logan-keede -- I think this is a great step forward. I think it would be better if we avoided moving the unit tests if possible, but I don't think that is a blocker strictly speaking

Love to see this project starting!

use std::sync::Arc;

pub use self::url::ListingTableUrl;
pub use datafusion_catalog_listing::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@@ -46,6 +46,8 @@ mod physical_optimizer;

mod catalog;

mod catalog_listing;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


# DataFusion catalog-listing

[DataFusion][df] is an extensible query execution framework, written in Rust, that uses Apache Arrow as its in-memory format.
Copy link
Contributor

Choose a reason for hiding this comment

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

I pushed a commit to improve this README content

@@ -569,218 +580,6 @@ mod tests {
assert_eq!(0, chunks.len());
}

#[tokio::test]
async fn test_pruned_partition_list_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Theses tests look like unit tests to me (they are are for pruned_partition_list which is defined in this module

I looked at make_test_store_and_state and it seems to me like it is fairly simple and could be copy/pasted into this test module to avoid a new dependency.

Would you be ok with doing that? I can also push a commit to this PR if it is good with you

Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty of doing this in 53dbc2f

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.

Let's go 🚀

@alamb alamb merged commit fe8ab01 into apache:main Feb 5, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants