Copilot commented on code in PR #50329: URL: https://github.com/apache/arrow/pull/50329#discussion_r3518316787
########## dev/archery/archery/crossbow/cli.py: ########## @@ -300,6 +300,10 @@ def asset_callback(task_name, task, asset): @crossbow.command() [email protected]('--base-branch', default='main', + help='Set base branch for the PR.') [email protected]('--head-branch', default=None, + help='Set head branch for the PR.') Review Comment: Since `--head-branch` is now used for API filtering, it would help to clarify in the CLI help text that GitHub may require `owner:branch` format (not just `branch`) to match PRs reliably. ########## cpp/src/arrow/compute/kernels/vector_select_k.cc: ########## @@ -118,22 +117,22 @@ void HeapSortNonNullsToOutput(std::span<uint64_t> non_null_input_range, Comparat return; } std::span<uint64_t> heap = non_null_input_range.subspan(0, output_range.size()); - std::ranges::make_heap(heap, cmp); + std::make_heap(heap.begin(), heap.end(), cmp); std::span<uint64_t> remaining_input = non_null_input_range.subspan(output_range.size()); for (uint64_t x_index : remaining_input) { if (cmp(x_index, heap.front())) { - std::ranges::pop_heap(heap, cmp); + std::pop_heap(heap.begin(), heap.end(), cmp); heap.back() = x_index; - std::ranges::push_heap(heap, cmp); + std::push_heap(heap.begin(), heap.end(), cmp); } } // fill output in reverse when destructing, // as the "worst" (next-to-would-have-been-replaced) element is at heap-top - 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(); i > 0; --i) { + output_range[i - 1] = heap.front(); // heap-top has the next element + std::pop_heap(heap.begin(), heap.end(), cmp); // Decrease heap-size by one heap = heap.first(heap.size() - 1); } Review Comment: This loop implicitly converts `output_range.size()` (size_t) to `int64_t`. Using a size_t reverse loop avoids signed conversion and is safer if sizes ever exceed `INT64_MAX`. ########## cpp/src/arrow/compute/kernels/vector_select_k.cc: ########## @@ -422,9 +421,8 @@ class ChunkedArraySelector : public TypeVisitor { // so the heap must have been completely filled DCHECK_EQ(heap.size(), output.non_null_like_range.size()); - for (uint64_t& reverse_out_iter : - std::ranges::reverse_view(output.non_null_like_range)) { - reverse_out_iter = + for (int64_t i = output.non_null_like_range.size(); i > 0; --i) { + output.non_null_like_range[i - 1] = heap.top().index + heap.top().offset; // heap-top has the next element heap.pop(); } Review Comment: Same signed-conversion issue here: `output.non_null_like_range.size()` is size_t but is assigned to `int64_t`. A size_t reverse loop avoids narrowing/signedness pitfalls. ########## dev/release/03-binary-submit.sh: ########## @@ -57,5 +58,7 @@ job_name=$(archery crossbow latest-prefix --no-fetch ${crossbow_job_prefix}) archery crossbow report-pr \ --no-fetch \ --arrow-remote "https://github.com/${ARROW_REPOSITORY}"; \ + --base-branch ${maint_branch} \ + --head-branch ${rc_branch} \ Review Comment: `archery crossbow report-pr` now filters PRs by `head`/`base`. GitHub’s `head` filter typically expects the `owner:branch` form; passing only `${rc_branch}` may cause the PR lookup to fail and prevent the tracking comment from being posted. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
