-
Notifications
You must be signed in to change notification settings - Fork 17
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
Added record bact import from/export to struct_array #336
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
==========================================
- Coverage 92.08% 90.41% -1.68%
==========================================
Files 83 83
Lines 6291 6455 +164
==========================================
+ Hits 5793 5836 +43
- Misses 498 619 +121
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
include/sparrow/record_batch.hpp
Outdated
std::transform( | ||
m_array_list.cbegin(), | ||
m_array_list.cend(), | ||
m_name_list.begin(), | ||
[](const array& ar) | ||
{ | ||
return ar.name(); | ||
} | ||
); | ||
init_array_map(); |
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.
Can we move these logic to dedicated functions which returns the values to put in the members ?
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.
I'm not sure we want to do that, this would require an additional memory allocation instead of filling the members in place. Besides, this transform is used only here. What is the issue with this implementation?
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.
IMO it's cleaner to have only a constructor initialization list.
std::vector<name_type> get_names(const std::vector<array>& array_list)
{
const auto names = array_list | std::views::transform(
[](const array& ar)
{
return ar.name();
}
return std::vector<name_type> {names.begin(), names.end()};
}
template <std::ranges::input_range CR>
requires std::same_as<std::ranges::range_value_t<CR>, array>
record_batch::record_batch(CR&& columns)
: m_array_list(to_vector<array>(std::move(columns)))
, m_name_list(get_names(m_array_list))
,
{
init_array_map();
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.
Oh indeed, I did not get that.
Co-authored-by: Alexis Placet <alexis.placet@gmail.com>
Co-authored-by: Alexis Placet <alexis.placet@gmail.com>
54a3f65
to
47ad140
Compare
7452e85
to
43a9f58
Compare
662f50c
to
ec606fe
Compare
for more information, see https://pre-commit.ci
No description provided.