limit: Fix HeadQuery + SlowFieldSort applying limit before sort#6690
limit: Fix HeadQuery + SlowFieldSort applying limit before sort#6690Maxr1998 wants to merge 1 commit into
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression test and updates DBCore iteration so that “slow sort” results are sorted before applying order-sensitive limits (e.g., HeadQuery), fixing incorrect top-N behavior when sorting by flexible (non-DB) fields.
Changes:
- Add a regression test covering limit prefix + slow (flexible-field) sorting behavior.
- Adjust
Results.__iter__slow-sort path to applyHeadQueryafter sorting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/plugins/test_limit.py | Adds regression test verifying correct top-N ordering when slow sort is used. |
| beets/dbcore/db.py | Changes slow-sort iteration to sort first, then apply order-sensitive query filtering. |
| # When a slow query is also present (e.g. HeadQuery), | ||
| # apply it AFTER sorting: | ||
| # slow queries may be order-sensitive (HeadQuery counts items and stops at N), | ||
| # so limiting before sorting would produce wrong results. | ||
| # Temporarily suppress the slow query to materialize all rows, sort them, then filter. | ||
| if self.query: | ||
| slow_query = self.query | ||
| self.query = None | ||
| try: | ||
| all_objects = list(self._get_objects()) | ||
| finally: | ||
| self.query = slow_query | ||
| sorted_objects = self.sort.sort(all_objects) | ||
| return iter( | ||
| obj for obj in sorted_objects if slow_query.match(obj) | ||
| ) |
There was a problem hiding this comment.
Good point. Keeping a list of order-sensitive query types is error-prone, though, so I'm unsure what the best approach is here.
| slow_query = self.query | ||
| self.query = None | ||
| try: | ||
| all_objects = list(self._get_objects()) | ||
| finally: | ||
| self.query = slow_query |
There was a problem hiding this comment.
I doubt thread-safety hazards really apply here, but adding the query as a parameter here should work. Let me know whether I should do that.
When `Results.__iter__()` has both a slow sort and a slow query active, the slow query was applied during row materialization (inside `_get_objects()`), before `sort.sort()` ran. Because `HeadQuery` is order-sensitive (it counts items and stops at N), it was capping the result set to N unsorted rows, which were then sorted. The fix temporarily suppresses the slow query while fetching all rows, sorts them, and only then applies the query to the sorted sequence. A regression test is added in `test/plugins/test_limit.py`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6690 +/- ##
==========================================
+ Coverage 72.57% 72.58% +0.01%
==========================================
Files 162 162
Lines 20810 20818 +8
Branches 3292 3293 +1
==========================================
+ Hits 15103 15111 +8
Misses 4982 4982
Partials 725 725
🚀 New features to boost your workflow:
|
Description
When
Results.__iter__()has both a slow sort and a slow query active, the slow query was applied during row materialization (inside_get_objects()), beforesort.sort()ran.Because
HeadQueryis order-sensitive (it counts items and stops at N), it was capping the result set to N unsorted rows, which were then sorted. The fix temporarily suppresses the slow query while fetching all rows, sorts them, and only then applies the query to the sorted sequence.A regression test is added in
test/plugins/test_limit.py.Fixes #5076.
To Do
Documentation.