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

moving memory.rs out of datafusion/core #14332

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

logan-keede
Copy link
Contributor

Which issue does this PR close?

Closes part of #10782

Rationale for this change

What changes are included in this PR?

moved memory.rs from datafusion/core/catalog_common to datafusion/catalog

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) catalog Related to the catalog crate labels Jan 28, 2025
//! Interfaces and default implementations of catalogs and schemas.
//!
//! Implementations
//! * Simple memory based catalog: [`MemoryCatalogProviderList`], [`MemoryCatalogProvider`], [`MemorySchemaProvider`]
Copy link
Contributor Author

@logan-keede logan-keede Jan 28, 2025

Choose a reason for hiding this comment

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

I am not sure if this is the correct place/way to put this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this looks good

One way to review this is to run cargo --doc --open and then looking at how it is rendered

@logan-keede
Copy link
Contributor Author

@alamb can this please get a review?
Thanks,
Logan

Copy link
Contributor

@comphead comphead 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'm thinking we should rename memory.rs to memory_catalog_provider.rs otherwise its really confusing, I thought originally its memory pool related crate but it is catalog related.

WDYT?

@logan-keede
Copy link
Contributor Author

Thanks @logan-keede I'm thinking we should rename memory.rs to memory_catalog_provider.rs otherwise its really confusing, I thought originally its memory pool related crate but it is catalog related.

WDYT?

Welcome, I think memory_catalog_provider.rs is too specific given that the same file also covers MemorySchemaProvider, given that similar pattern is being followed by almost all the files(look at async.rs), we can assume that any developer working on that set of file will notice it too.
However, if you think that might cause inefficiency we can go with something like memory_providers, again, for consistency's sake if we go with that we should change name of every similar file in a similar manner (atleast async.rs).

I am not sure if this is the correct place/way to put this.

any idea/comment?

@logan-keede logan-keede requested a review from comphead January 29, 2025 13:26
@comphead
Copy link
Contributor

if we go with that we should change name of every similar file in a similar manner (atleast async.rs).

That is true, to be consistent it needs to be fixed for all files which is not part of this PR for sure

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @logan-keede

@comphead comphead merged commit 1da5252 into apache:main Jan 29, 2025
27 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 29, 2025

😍

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.

Thank you @logan-keede and @comphead ❤️

//! Interfaces and default implementations of catalogs and schemas.
//!
//! Implementations
//! * Simple memory based catalog: [`MemoryCatalogProviderList`], [`MemoryCatalogProvider`], [`MemorySchemaProvider`]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this looks good

One way to review this is to run cargo --doc --open and then looking at how it is rendered

@comphead
Copy link
Contributor

One way to review this is to run cargo --doc --open and then looking at how it is rendered

Oh this is much easier way than I kept doing before, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
catalog Related to the catalog crate core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants