-
Notifications
You must be signed in to change notification settings - Fork 19
Nullable in cosntructors, set NULLABLE flag when an element is null or if the array was created nullable #380
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #380 +/- ##
==========================================
- Coverage 89.78% 89.74% -0.04%
==========================================
Files 93 93
Lines 6941 7121 +180
==========================================
+ Hits 6232 6391 +159
- Misses 709 730 +21
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:
|
6b4bf0a
to
4049145
Compare
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.
Copilot reviewed 35 out of 36 changed files in this pull request and generated no comments.
Files not reviewed (1)
- CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (4)
include/sparrow/buffer/dynamic_bitset/dynamic_bitset_base.hpp:198
- Resizing the dynamic_bitset in operator[] when data() is nullptr may lead to unexpected side effects. Consider documenting this behavior or handling a null data pointer in a more explicit manner.
if (data() == nullptr) { resize(m_size, true); }
include/sparrow/arrow_interface/arrow_schema.hpp:111
- Aggregating flag values using bitwise OR assumes that multiple flags can be combined in this manner. Verify that this approach correctly represents the combined state for all intended flags.
for (const auto& flag : *flags) { schema.flags |= static_cast<int64_t>(flag); }
include/sparrow/layout/list_layout/list_array.hpp:592
- The use of a static flag set in one branch and std::nullopt in the other may lead to inconsistent flag representation across list array proxies. Ensure the flag usage is consistent between the nullable and non-nullable branches.
static const std::optional<std::unordered_set<ArrowFlag>> flags{{ArrowFlag::NULLABLE}};
include/sparrow/buffer/dynamic_bitset/dynamic_bitset_base.hpp:223
- The check for m_buffer being nullptr in the test() function contrasts with the behavior in operator[]. Ensure that the handling of null storage pointers is consistent and well-documented across all functions.
if (m_buffer == nullptr) { return true; }
05aaf80
to
67135e2
Compare
79f25dc
to
39aa2dc
Compare
for more information, see https://pre-commit.ci
@@ -407,7 +405,7 @@ namespace sparrow | |||
auto array_crtp_base<D>::has_value(size_type i) const -> bitmap_const_reference | |||
{ | |||
SPARROW_ASSERT_TRUE(i < size()); | |||
return *sparrow::next(bitmap_begin(), i); | |||
return this->derived_cast().get_bitmap()[i + get_arrow_proxy().offset()]; |
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.
Why this change? The previous version was easier to read.
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, fixed
[[nodiscard]] static auto create_proxy( | ||
u8_buffer<storage_type>&& data_buffer, | ||
std::size_t precision, | ||
int scale, | ||
NULLABLE_TYPE nullable = true, |
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 is the issue with directly using bool
instead of NULLABLE_TYPE
here? Same question for the other create_proxy mathods below since it looks to be a pattern.
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, fixed
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
No description provided.