GH-50636: [C++] Replace std::span/ranges usage to fix macOS CRAN - #50705
Conversation
|
@github-actions crossbow submit test-r-macos-as-cran |
|
Revision: 78bd740 Submitted crossbow builds: ursacomputing/crossbow @ actions-15e03f43e9
|
|
Now [ 87%] Building CXX object src/parquet/CMakeFiles/parquet_objlib.dir/arrow/reader.cc.o
/Users/runner/work/crossbow/crossbow/arrow/cpp/src/parquet/arrow/reader.cc:735:46: error: no member named 'ranges' in namespace 'std'
735 | const auto first_invalid_offset = std::ranges::adjacent_find(
| ~~~~~^
/Users/runner/work/crossbow/crossbow/arrow/cpp/src/parquet/arrow/reader.cc:740:26: error: no member named 'ranges' in namespace 'std'
740 | start + std::ranges::distance(run_offsets.begin(), first_invalid_offset);
| ~~~~~^
2 errors generated.
make[2]: *** [src/parquet/CMakeFiles/parquet_objlib.dir/arrow/reader.cc.o] Error 1from yesterday's #50271 |
|
@github-actions crossbow submit test-r-macos-as-cran |
|
Revision: 0b69409 Submitted crossbow builds: ursacomputing/crossbow @ actions-59cf634a38
|
tadeja
changed the title
|
With fix in |
| size_t range_start = 0; | ||
| size_t range_cur = 0; | ||
| auto last_value = GetView::LogicalValue(array.GetView(indices[range_cur] - offset)); | ||
| while (++range_cur != indices.size()) { | ||
| auto v = GetView::LogicalValue(array.GetView(indices[range_cur] - offset)); | ||
| if (v != last_value) { | ||
| visit({range_start, range_cur}); | ||
| visit(indices.subspan(range_start, range_cur - range_start)); | ||
| range_start = range_cur; | ||
| last_value = v; | ||
| } | ||
| } | ||
| if (range_start != range_cur) { | ||
| visit({range_start, range_cur}); | ||
| visit(indices.subspan(range_start, range_cur - range_start)); |
There was a problem hiding this comment.
This looks very sensible. Good idea to revert to size_t entirely instead of recalculating (start, size) using iterator differences in the subspan calls!
Thank you, I was not aware of the portability problems re/ std::span!
github-actions
Bot
added
awaiting committer review
Sad. I didn't expect there are still some failing CI build. Thank you for your fix. I updated #48587 (comment) to document this. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 14acca2. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
4 participants
Rationale for this change
Fix #50636 -
test-r-macos-as-crannightly job fails compilingvisit({range_start, range_cur})introduced in #50248.The job pins macOS SDK 11.3, so libc++ there does not have C++20 iterator-pair span constructor available yet (available with libc++ 14).
Similar problem as in recent #50295
What changes are included in this PR?
Replace std::span iterator-pair constructor with subspan in
vector_sort.ccAlso replace std::ranges in
parquet/arrow/reader.ccintroduced in #50271Are these changes tested?
Yes, builds locally and crossbow
test-r-macos-as-cranjob succeeds.Are there any user-facing changes?
No.