-
Notifications
You must be signed in to change notification settings - Fork 183
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
Array Metadata related refactoring, use Serializer/Deserializer instead of Buffer #3544
Array Metadata related refactoring, use Serializer/Deserializer instead of Buffer #3544
Conversation
…ager::store_metadata, StorageManager::load_array_metadata and related
This pull request has been linked to Shortcut Story #19506: Remove buffer from array metadata serialization/deserialization.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of very small, easily addressable comments. And some suggestions to convert some methods to use exceptions instead of Status when it's easy.
tiledb/sm/metadata/CMakeLists.txt
Outdated
@@ -50,6 +50,8 @@ if (TILEDB_TESTS) | |||
find_package(Catch_EP REQUIRED) | |||
target_link_libraries(unit_metadata PUBLIC Catch2::Catch2) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: remove space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* | ||
* @return remaining unread bytes in deserializer. | ||
*/ | ||
storage_size_t remaining_bytes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark method const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
return Status::Ok(); | ||
}); | ||
RETURN_NOT_OK(status); | ||
|
||
// Compute array metadata size for the statistics | ||
uint64_t meta_size = 0; | ||
for (const auto& b : metadata_buffs) | ||
meta_size += b->size(); | ||
for (const auto& t : metadata_tiles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add brackets to get rid of single line for loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -2246,32 +2245,28 @@ Status StorageManager::load_group_metadata( | |||
const auto& group_metadata_to_load = group_dir->group_meta_uris(); | |||
|
|||
auto metadata_num = group_metadata_to_load.size(); | |||
std::vector<shared_ptr<Buffer>> metadata_buffs; | |||
metadata_buffs.resize(metadata_num); | |||
std::vector<shared_ptr<Tile>> metadata_tiles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The good thing about using a vector of shared_ptr is that we don't get default constructed Tile objects here. We could use DynamicArray, but it's not quite ready for prime time and also doesn't destroy objects once it's done. Maybe add a comment above to mention we could move this to DynamicArray at some point. Also we could combine the two lines in std::vector<shared_ptr> metadata_tiles(metadata_num);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made guess at comment you desire,
combined initial allocation into construction.
@@ -1660,31 +1660,27 @@ Status StorageManager::load_array_metadata( | |||
const auto& array_metadata_to_load = array_dir.array_meta_uris(); | |||
|
|||
auto metadata_num = array_metadata_to_load.size(); | |||
std::vector<shared_ptr<Buffer>> metadata_buffs; | |||
metadata_buffs.resize(metadata_num); | |||
std::vector<shared_ptr<Tile>> metadata_tiles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine two lines into std::vector<shared_ptr> metadata_tiles(metadata_num);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
stats_->add_counter("read_array_meta_size", meta_size); | ||
|
||
*metadata = Metadata::deserialize(metadata_buffs); | ||
*metadata = Metadata::deserialize(metadata_tiles); | ||
|
||
// Sets the loaded metadata URIs | ||
RETURN_NOT_OK(metadata->set_loaded_metadata_uris(array_metadata_to_load)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably convert set_loaded_metadata_uris to return void.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Quick comment, could we update the title to mention Array Metadata? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
replace Buffer usage with Serializer/Deserializer in items StorageManager::store_metadata, StorageManager::load_array_metadata and related
TYPE: IMPROVEMENT
DESC: Array Metadata related refactoring, use Serializer/Deserializer instead of Buffer