Improve struct array and value #535
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
+ Coverage 87.97% 88.21% +0.24%
==========================================
Files 100 100
Lines 7649 7697 +48
==========================================
+ Hits 6729 6790 +61
+ Misses 920 907 -13
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:
|
Alex-PLACET
force-pushed
the
improve_struct_array_and_value
branch
from
bcef271 to
681abd9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR improves the struct array and struct value implementation by adding name support for child arrays, bounds-checked access methods, and array manipulation capabilities.
- Added name support to primitive arrays and struct values, allowing fields to be accessed by name
- Implemented bounds-checked
at()methods for struct values with both index and name-based access - Added array manipulation methods (
add_child,set_child,pop_children) to enable dynamic modification of struct arrays
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_struct_array.cpp | Added comprehensive tests for new functionality including named field access and array manipulation methods |
| src/struct_array.cpp | Implemented pop_children method and optimized iterator constructors |
| src/layout/struct_value.cpp | Added bounds-checked at() methods and name-based field access |
| src/arrow_interface/arrow_array_schema_proxy.cpp | Implemented set_child methods for replacing child arrays |
| include/sparrow/struct_array.hpp | Added public interface methods for array manipulation and name access |
| include/sparrow/layout/struct_value.hpp | Extended struct_value interface with at() methods and names() accessor |
Comments suppressed due to low confidence (2)
test/test_struct_array.cpp:75
- Typo in test case name: 'whildren' should be 'children'.
SUBCASE("with whildren, nullable, name and metadata")
test/test_struct_array.cpp:131
- Typo in test case name: 'whildren' should be 'children'.
SUBCASE("with whildren, bitmap, name and metadata")
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
for more information, see https://pre-commit.ci
include/sparrow/struct_array.hpp
Outdated
| SPARROW_ASSERT_TRUE(child.size() == size()); | ||
| auto [array, schema] = extract_arrow_structures(std::forward<A>(child)); | ||
| get_arrow_proxy().set_child(index, std::move(array), std::move(schema)); | ||
| m_children = make_children(); |
There was a problem hiding this comment.
Can we avoid rebuilding the whole children list, and just rebuild the one we changed?
|
|
||
| auto struct_value::operator[](size_type i) const -> const_reference | ||
| { | ||
| SPARROW_ASSERT_TRUE(i < size()); |
There was a problem hiding this comment.
I agree with copilot, this should be removed, or we should change the definition of https://github.com/man-group/sparrow/blob/main/include/sparrow/utils/contracts.hpp#L172 and set it to 0 by default (and to 1 when building in DEBUG).
There was a problem hiding this comment.
We have this assert in all the other classes
There was a problem hiding this comment.
Ah indeed, that should be addressed in a dedicated PR
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| if constexpr (std::is_same_v<std::decay_t<decltype(val1)>, nullable_uint8_t>) | ||
| { | ||
| CHECK_EQ(val1.value(), static_cast<inner_scalar_type>(i)); |
There was a problem hiding this comment.
The cast to inner_scalar_type is incorrect here. The variable val1 is of type nullable_uint8_t (as shown in line 279), so it should be cast to std::uint8_t instead of inner_scalar_type.
| CHECK_EQ(val1.value(), static_cast<inner_scalar_type>(i)); | |
| CHECK_EQ(val1.value(), static_cast<std::uint8_t>(i)); |
Copilot uses AI. Check for mistakes.
| { | ||
| SPARROW_ASSERT_TRUE(child.size() == size()); | ||
| } | ||
| m_children.reserve(m_children.size() + children.size()); |
There was a problem hiding this comment.
The children.size() call may be inefficient for input ranges that don't have constant-time size computation. Consider using std::ranges::distance or iterating without pre-reservation for generic input ranges.
| m_children.reserve(m_children.size() + children.size()); | |
| m_children.reserve(m_children.size() + std::ranges::distance(children)); |
Copilot uses AI. Check for mistakes.
2 participants
No description provided.