-
Notifications
You must be signed in to change notification settings - Fork 4
Attempt to fix MSVC failure #66
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
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
=======================================
Coverage ? 82.45%
=======================================
Files ? 36
Lines ? 1755
Branches ? 0
=======================================
Hits ? 1447
Misses ? 308
Partials ? 0
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:
|
This reverts commit 79de94f.
|
After checking the different places where we use @Alex-PLACET if this sounds good to you I will merge this PR, to be reverted when this is fixed upstream on |
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 think the error comes from comparing iterators from two different temporary views. If the suggested fix works, we should probably improve sparrow documentation to avoid this bad surprise.
| const auto& arrow_proxy = sparrow::detail::array_access::get_arrow_proxy(arr); | ||
| return acc + calculate_body_size(arrow_proxy, compression, cache); | ||
| } | ||
| ); |
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.
Would it work to instantiate the temporary view returned by record_batch only once before using it in std::accumulate? I.e. doing something like:
auto cols = record_batch.columns();
return std::accumulate(cols.begin(), cols.end(), ....);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.
We could also introduce a version of accumulate that accepts a range instead of a pair of iterators.
src/serialize_utils.cpp
Outdated
| std::optional<std::reference_wrapper<CompressionCache>> cache) | ||
| { | ||
| std::for_each(record_batch.columns().begin(), record_batch.columns().end(), [&](const auto& column) { | ||
| for (const auto& column : record_batch.columns()) |
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.
std::ranges::for_each(record_batch.columns(), ...) should fix the issue.
This is fixing the MSVC failure on windows builds (from conda-forge and on debug).
The error is the following:
This happened once the CI started using
sparrow-devel 1.4.0, specifically due to changes in this PR whererecord_batch::columns()is now usingstd::views::transform.I don't know the rationale behind this change, specifically whether it involves using
std::views::transformto handle record batch arrays as references, or whether we could change this approach.If the latter is true, maybe add a
TODOnote here to change it back to how things were handled.