Repository navigation
refactor: Move Memtable to catalog - #15459
Conversation
| } | ||
|
|
||
| #[async_trait] | ||
| impl DataSink for MemSink { |
There was a problem hiding this comment.
I think MemSink should be in datasource crate
There was a problem hiding this comment.
Unless you would specifically like to make it public (which is also reasonable).
I think it is fine to have it in catalog because MemSink is a private struct used only by MemTable even though it logically makes more sense to have it with its DataSource implementation to like other DataSink impls.
I have pushed a commit feel free to drop it
There was a problem hiding this comment.
I agree with Jay. We shouldn't try to push every piece of code to the higher level crates, as maintaining integrity and logical structure is more important to keep each crate meaningful.
|
Maybe as a follow on PR we can make a new crate Something I noticed recently while building datafusion is that compiling the catalog crate itself takes non trivial time too (7s on my laptop as I recall) I left more notes here: #13814 (comment) |
* first iteration * fix: CI * move statistics * Merge relics * MemSink to datasource * clean stuff + backward compatibility * fix: cargo doc * fix:fmt
Which issue does this PR close?
datafusioncrate (datafusion/core) #14444Rationale for this change
now that
catalogis dependent ondatasource,MemTablecan go to catalog.What changes are included in this PR?
move In-Memory Table Provider to
catalog.and statistics to datasource.
Are these changes tested?
By Github CI.
Are there any user-facing changes?
you can access
datafusion::datasource::memory::{DataSourceExec, MemorySourceConfig, Tableproviders.., CatalogProviders.., SchemaProviders..},Reason: