Skip to content

fix(rust): Incorrect arg_sort with descending+limit#26839

Merged
nameexhaustion merged 3 commits intopola-rs:mainfrom
boris324:fix/arg-sort-descending-limit
Mar 10, 2026
Merged

fix(rust): Incorrect arg_sort with descending+limit#26839
nameexhaustion merged 3 commits intopola-rs:mainfrom
boris324:fix/arg-sort-descending-limit

Conversation

@boris324
Copy link
Contributor

@boris324 boris324 commented Mar 6, 2026

Summary

  • Fix arg_sort and arg_sort_no_nulls to use the correct comparator when descending=true and a limit is set
  • select_nth_unstable_by was always using ascending comparison to partition elements, causing the smallest N elements to be selected instead of the largest N when descending=true
  • Add tests for arg_sort with descending+limit, ascending+limit, and descending+limit+nulls

Closes #26833

Test plan

  • Added test_arg_sort_descending_with_limit - verifies top-3 largest indices are returned
  • Added test_arg_sort_asc_with_limit - verifies top-3 smallest indices (regression guard)
  • Added test_arg_sort_desc_limit_nulls - verifies descending+limit with null values

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars first-contribution First contribution by user title needs formatting labels Mar 6, 2026
When arg_sort is called with descending=true and a limit, select_nth_unstable_by
was always using the ascending comparator to partition elements. This caused
the wrong N elements to be selected (smallest instead of largest).

Reverse the comparator when descending is set in both arg_sort and
arg_sort_no_nulls functions.

Closes pola-rs#26833

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@boris324 boris324 force-pushed the fix/arg-sort-descending-limit branch from 8a713ba to 91f7251 Compare March 6, 2026 23:07
@boris324 boris324 changed the title fix: use correct comparator for arg_sort with descending + limit fix: arg_sort with descending+limit selects wrong elements Mar 6, 2026
@codecov
Copy link

codecov bot commented Mar 7, 2026

Codecov Report

❌ Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.68%. Comparing base (9d60226) to head (d3601c4).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...polars-core/src/chunked_array/ops/sort/arg_sort.rs 96.07% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26839      +/-   ##
==========================================
+ Coverage   81.00%   81.68%   +0.68%     
==========================================
  Files        1805     1805              
  Lines      248021   248066      +45     
  Branches     3132     3132              
==========================================
+ Hits       200902   202635    +1733     
+ Misses      46313    44625    -1688     
  Partials      806      806              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nameexhaustion nameexhaustion changed the title fix: arg_sort with descending+limit selects wrong elements fix(rust): Incorrect arg_sort with descending+limit Mar 10, 2026
@nameexhaustion nameexhaustion merged commit f110a45 into pola-rs:main Mar 10, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

first-contribution First contribution by user fix Bug fix python Related to Python Polars RC rust Related to Rust Polars

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SortOption.limit combined with descending=True does not work for arg_sort

2 participants