Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request consolidates storage management types by removing redundant abstractions. The main change unifies all storage operations under modelardb_storage::DataFolder, eliminating separate types like modelardb_embedded::DataFolder, TableMetadataManager, and MetadataManager. The PR also reduces multiple SessionContext instances to a single one managed by DataFolder, simplifying the architecture while maintaining all existing functionality.
Key Changes:
- Removed
DeltaLakeandTableMetadataManagertypes; their functionality now resides inDataFolder - Consolidated all session contexts into a single instance within
DataFolder - Metadata tables now use a
metadataschema prefix for clearer organization
Reviewed Changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/modelardb_storage/src/data_folder.rs | New unified DataFolder implementation combining Delta Lake and metadata management |
| crates/modelardb_storage/src/delta_lake.rs | Removed (functionality moved to DataFolder) |
| crates/modelardb_storage/src/metadata/table_metadata_manager.rs | Removed (functionality moved to DataFolder) |
| crates/modelardb_storage/src/metadata/mod.rs | Removed |
| crates/modelardb_storage/src/lib.rs | Updated module exports and added metadata schema registration |
| crates/modelardb_server/src/storage/uncompressed_data_manager.rs | Updated to use unified DataFolder API |
| crates/modelardb_server/src/context.rs | Removed dedicated SessionContext; now accessed via DataFolder |
| crates/modelardb_manager/src/metadata.rs | Converted to trait-based approach with ManagerMetadata |
| crates/modelardb_embedded/src/operations/data_folder.rs | Removed embedded DataFolder implementation |
| crates/modelardb_compression/src/compression.rs | Added multivariate compression functions |
| crates/modelardb_compression/src/lib.rs | Updated public API exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CGodiksen
requested changes
Oct 29, 2025
CGodiksen
approved these changes
Oct 30, 2025
chrthomsen
approved these changes
Oct 31, 2025
e013587 to
ef9c74d
Compare
This was referenced Nov 2, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes all types for managing storage except for
modelardb_storageDataFolder. Thus,modelardb_embeddedDataFolder,TableMetadataManager,MetadataManager, etc have been removed. In addition, the multiple instances ofSessionContextthat were used have been reduced to a singleSessionContextthat is part ofmodelardb_storageDataFolder. Thus,modelardb_storageDataFoldernow represents a location that stores data and can query data, and all methods for operating on data at that location are part of it. An attempt to splitmodelardb_storageDataFolderinto two types was made, where one type handles files and one provides abstractions on top of files in the form of tables. However, as the dependency between the instance variables and methods of these two types made implementing them very complex, and most operations are already on tables due todeltalake, it was decided to simply implement data storage and querying asmodelardb_storageDataFolder. There is still some overlap between the methods inContextandmodelardb_storageDataFolder, e.g., for registering tables, but removing the duplicate code seems to require significant changes due to the difference between howDataSinkis used inmodelardb_serverandmodelardb_embedded. Likewise, the methods for handling normal tables and metadata tables inmodelardb_storageDataFoldercan probably be reduced to one set of methods. However, since the purpose of this PR was to reduce the number of types for managing storage to one, the PR was already very big, and there are issues for reducing the number of methods for specific table types, is simplifying and combining methods purposely not part of this PR.