-
Notifications
You must be signed in to change notification settings - Fork 21
Add mutability binary view #526
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
Add mutability binary view #526
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #526 +/- ##
==========================================
+ Coverage 88.27% 88.44% +0.17%
==========================================
Files 100 100
Lines 7709 7886 +177
==========================================
+ Hits 6805 6975 +170
- Misses 904 911 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR adds mutation support to the variable size binary view array implementation, enabling resize, insert, and erase operations. The implementation provides comprehensive mutating functionality while managing the complex storage layout of binary view arrays with their inline storage optimization for strings ≤12 bytes.
Key changes:
- Adds assignment, resize, insert, and erase methods to the binary view array
- Implements comprehensive test coverage for all mutation operations
- Fixes assertion logic in the base class to allow empty range operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
include/sparrow/variable_size_binary_view_array.hpp |
Adds mutating methods (assign, resize_values, insert_value, insert_values, erase_values) with complex buffer management logic |
include/sparrow/layout/mutable_array_base.hpp |
Fixes assertion to allow empty range erases (first == last) |
test/test_variable_size_binary_view_array.cpp |
Comprehensive test suite covering all mutation scenarios including edge cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
*dest = static_cast<std::uint8_t>(*src_it); | ||
++src_it; | ||
++dest; | ||
} |
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.
What about simplifying the implementation with something like:
std::ranges::transform(range, dest, to_uint8_transform());
And since it's a single line, I think calls to copy_and_convert_to_uint8
could actually be replaced with that single line.
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
++src_it; | ||
++dest; | ||
++copied; | ||
} |
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.
This implementation could be simplified with something like:
std::ranges::transform(range | std::views::take(count), dest, to_uint8_transform());
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
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
ccf0660
to
c90061e
Compare
for more information, see https://pre-commit.ci
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.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
std::size_t write_offset = 0; | ||
|
||
// Create mapping of old offsets to new offsets | ||
std::unordered_map<std::size_t, std::size_t> offset_mapping; |
Copilot
AI
Oct 17, 2025
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.
Using std::unordered_map for offset mapping in the erase operation could be inefficient for arrays with many long strings. Consider using a sorted vector of pairs with binary search, or pre-allocate the map with an appropriate bucket count.
std::unordered_map<std::size_t, std::size_t> offset_mapping; | |
std::unordered_map<std::size_t, std::size_t> offset_mapping; | |
offset_mapping.reserve(current_size - count); |
Copilot uses AI. Check for mistakes.
auto transformed = rhs | ||
| std::ranges::views::transform( | ||
transform_to<typename T::value_type, typename T::value_type> | ||
); | ||
|
||
std::ranges::copy(transformed, reinterpret_cast<typename T::value_type*>(data_ptr)); | ||
|
Copilot
AI
Oct 17, 2025
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 transform operation from T::value_type to T::value_type is redundant. This identity transformation adds unnecessary overhead and should be removed.
auto transformed = rhs | |
| std::ranges::views::transform( | |
transform_to<typename T::value_type, typename T::value_type> | |
); | |
std::ranges::copy(transformed, reinterpret_cast<typename T::value_type*>(data_ptr)); | |
// Directly copy rhs into the data pointer, as transform is redundant | |
std::ranges::copy(rhs, reinterpret_cast<typename T::value_type*>(data_ptr)); |
Copilot uses AI. Check for mistakes.
…PLACET/sparrow into add_mutability_binary_view
for more information, see https://pre-commit.ci
No description provided.