Skip to content

GH-50295: [C++][R] #include <ranges> in vector_select_k.cc breaks macOS CRAN and wasm builds#50297

Merged
thisisnic merged 4 commits into
apache:mainfrom
thisisnic:GH-50295-ranges
Jul 1, 2026
Merged

GH-50295: [C++][R] #include <ranges> in vector_select_k.cc breaks macOS CRAN and wasm builds#50297
thisisnic merged 4 commits into
apache:mainfrom
thisisnic:GH-50295-ranges

Conversation

@thisisnic

@thisisnic thisisnic commented Jun 30, 2026

Copy link
Copy Markdown
Member

Rationale for this change

R build failures due CRAN toolchain

What changes are included in this PR?

Use version of functions available on CRAN toolchain

Are these changes tested?

By existing CI jobs, and additional unit tests.

Are there any user-facing changes?

No

Copilot AI review requested due to automatic review settings June 30, 2026 08:11
@thisisnic thisisnic requested a review from pitrou as a code owner June 30, 2026 08:11
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50295 has been automatically assigned in GitHub to PR creator.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50295 has no components, please add labels for components.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses R build failures on the macOS CRAN toolchain and in wasm/emscripten by removing the dependency on the C++20 <ranges> header in the C++ select_k kernel implementation.

Changes:

  • Removed #include <ranges> from vector_select_k.cc.
  • Replaced std::ranges::{make_heap,pop_heap,push_heap} with iterator-based std::{make_heap,pop_heap,push_heap} usage.
  • Replaced std::ranges::reverse_view(...) output filling with manual reverse-index loops.

Comment thread cpp/src/arrow/compute/kernels/vector_select_k.cc Outdated
@pitrou

pitrou commented Jun 30, 2026

Copy link
Copy Markdown
Member

cc @taepper

Comment on lines +424 to +425
for (int64_t i = output.non_null_like_range.size() - 1; i >= 0; --i) {
output.non_null_like_range[i] =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Copilot comment is right but the suggested solution is ugly. Could be something like:

Suggested change
for (int64_t i = output.non_null_like_range.size() - 1; i >= 0; --i) {
output.non_null_like_range[i] =
for (int64_t i = output.non_null_like_range.size(); i > 0; --i) {
output.non_null_like_range[i - 1] =

or:

Suggested change
for (int64_t i = output.non_null_like_range.size() - 1; i >= 0; --i) {
output.non_null_like_range[i] =
for (auto it = output.non_null_like_range.rbegin();
it != output.non_null_like_range.rend(); ++it) {
*it =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed tests for the all-nulls case in thisisnic#34 @thisisnic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers @pitrou

for (auto& reverse_out_iter : std::ranges::reverse_view(output_range)) {
reverse_out_iter = heap.front(); // heap-top has the next element
std::ranges::pop_heap(heap, cmp);
for (int64_t i = output_range.size() - 1; i >= 0; --i) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot didn't notice, but this has the same problem as below.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 30, 2026
@thisisnic thisisnic changed the title GH-50295: [C++][R] #include <ranges> in vector_select_k.cc breaks macOS CRAN and wasm builds # GH-50295: [C++][R] #include <ranges> in vector_select_k.cc breaks macOS CRAN and wasm builds Jun 30, 2026
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50295 has no components, please add labels for components.

Add tests for columns without non-null values
Copilot AI review requested due to automatic review settings June 30, 2026 09:29
@github-actions github-actions Bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50295 has no components, please add labels for components.

@pitrou

pitrou commented Jun 30, 2026

Copy link
Copy Markdown
Member

@thisisnic Do you think we can upgrade the CRAN and wasm builds at some point?

(also, which wasm build is it?)

@thisisnic

Copy link
Copy Markdown
Member Author

@thisisnic Do you think we can upgrade the CRAN and wasm builds at some point?

It's not a CI issue, the CI is up to date but it's CRAN who are using Apple Clang which apparently doesn't have ranges in its libc++

(also, which wasm build is it?)

The two failing nightlies:

Hmm, the wasm one might be for a different reason actually, I will check more.

@rok

rok commented Jul 1, 2026

Copy link
Copy Markdown
Member

Hmm, the wasm one might be for a different reason actually, I will check more.

#50317 perhaps?

@taepper

taepper commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Sorry for the troubles this caused (I saw it included blocking the creation of release candidates!)

@thisisnic

Copy link
Copy Markdown
Member Author

CI failures are unrelated, so I'll merge.

@thisisnic thisisnic merged commit f10c93c into apache:main Jul 1, 2026
60 of 62 checks passed
@thisisnic thisisnic removed the awaiting changes Awaiting changes label Jul 1, 2026
raulcd pushed a commit that referenced this pull request Jul 2, 2026
…OS CRAN and wasm builds (#50297)

### Rationale for this change

R build failures due CRAN toolchain

### What changes are included in this PR?

Use version of functions available on CRAN toolchain

### Are these changes tested?

By existing CI jobs, and additional unit tests.

### Are there any user-facing changes?

No
* GitHub Issue: #50295

Lead-authored-by: Nic Crane <thisisnic@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Nic Crane <thisisnic@gmail.com>
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit f10c93c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 12 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants