[c++] Implement buffer resize and resubmission for read queries #4375
Conversation
XanthosXanthopoulos
force-pushed
the
xan/SOMA-796
branch
from
d4fafd0 to
d13a15a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4375 +/- ##
==========================================
- Coverage 86.36% 86.36% -0.01%
==========================================
Files 139 139
Lines 21093 21093
Branches 15 15
==========================================
- Hits 18218 18216 -2
- Misses 2875 2877 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jp-dark
changed the title
alancleary
left a comment
alancleary
left a comment
There was a problem hiding this comment.
This is my first SOMA review so it's pretty limited in scope.
Overall everything looks good; just a couple minor change requests and some questions about the buffer's memory budget.
| */ | ||
| static bool use_memory_pool(const std::shared_ptr<tiledb::Array>& array); | ||
|
|
||
| void expand_buffers(); |
There was a problem hiding this comment.
Add a docstring comment explaining what the public method does.
| // Map: column name -> ColumnBuffer | ||
| std::unordered_map<std::string, std::shared_ptr<ColumnBuffer>> buffers_; | ||
|
|
||
| std::unique_ptr<ColumnBufferAllocationStrategy> strategy_; |
There was a problem hiding this comment.
Add a comment explaining this member for consistency
| void ArrayBuffers::expand_buffers() { | ||
| for (const auto& [name, buffer] : buffers_) { | ||
| buffer->resize( | ||
| buffer->max_size() * DEFAULT_BUFFER_EXPANSION_FACTOR, |
There was a problem hiding this comment.
Does this need to check if a memory budget is being exceeded?
There was a problem hiding this comment.
This call will exceed the buffer by default. The buffers initially allocated use all available budget by default so if the read didn't manage to return any results then it means that the memory budget was to small to begin with.
| /** | ||
| * @brief Resize the internal buffers to the given size. | ||
| */ | ||
| void resize(uint64_t size, uint64_t num_cells, bool preserve_data = false); |
There was a problem hiding this comment.
Parameters should be const when not modifying to facilitate compiler optimizations
There was a problem hiding this comment.
Also add @param descriptions to the docstring
| virtual ~ColumnBufferAllocationStrategy() = default; | ||
|
|
||
| virtual std::pair<size_t, size_t> get_buffer_sizes( | ||
| std::variant<tiledb::Attribute, tiledb::Dimension> column) const = 0; |
There was a problem hiding this comment.
column should be const since none of the implementations seem to modify it
| soma_array->close(); | ||
| } | ||
|
|
||
| TEST_CASE("SOMAArray: Test resize") { |
There was a problem hiding this comment.
Is there a way to constrain the buffer's memory budget? If so, this code should test that resizing beyond the budget throws an error.
Issue and/or context: SOMA 796
Changes:
This PR introduces the following changes:
ArrayBuffersgeneric.ColumnBufferpublic API to report maximum capacity for reads