-
Notifications
You must be signed in to change notification settings - Fork 457
fix: deterministic tiebreak for ranked file/dir results (#900) #903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,10 +164,7 @@ pub(crate) fn fuzzy_match_and_score_files<'a>( | |
|
|
||
| // Process overflow files first: newly added files (created after the | ||
| // initial scan) live in the overflow arena and are more likely to be | ||
| // relevant to the current search. | ||
| // | ||
| // putting them first in the list makes sorting more efficient and gives | ||
| // them tiebreaker advantage in case sorting is the same | ||
| // relevant to the current search. Exact ties are broken by index order. | ||
| let results = if files.len() > base_count { | ||
| let mut results = match_and_score_in_arena(&files[base_count..], context, overflow_arena); | ||
|
|
||
|
|
@@ -645,12 +642,17 @@ fn sort_and_paginate_dirs<'a>( | |
| let items_needed = offset.saturating_add(limit).min(total_matched); | ||
| let use_partial_sort = items_needed < total_matched / 2 && total_matched > 100; | ||
|
|
||
| let compare = |a: &(&DirItem, Score), b: &(&DirItem, Score)| { | ||
| b.1.total | ||
| .cmp(&a.1.total) | ||
| .then_with(|| std::ptr::from_ref(a.0).cmp(&std::ptr::from_ref(b.0))) | ||
| }; | ||
| if use_partial_sort { | ||
| results.select_nth_unstable_by(items_needed - 1, |a, b| b.1.total.cmp(&a.1.total)); | ||
| results.select_nth_unstable_by(items_needed - 1, compare); | ||
| results.truncate(items_needed); | ||
| } | ||
|
|
||
| sort_with_buffer(&mut results, |a, b| b.1.total.cmp(&a.1.total)); | ||
| sort_with_buffer(&mut results, compare); | ||
|
|
||
| let (items, scores): (Vec<&DirItem>, Vec<Score>) = | ||
| results.into_iter().skip(offset).take(limit).unzip(); | ||
|
|
@@ -1136,6 +1138,8 @@ fn sort_and_paginate<'a, S>( | |
| total(&b.1) | ||
| .cmp(&total(&a.1)) | ||
| .then_with(|| b.0.modified.cmp(&a.0.modified)) | ||
| // Total order: parallel matching yields scheduling-dependent input order. | ||
| .then_with(|| std::ptr::from_ref(a.0).cmp(&std::ptr::from_ref(b.0))) | ||
|
Comment on lines
+1141
to
+1142
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- exact PR diff ---'
git diff --unified=8 89c19270ea2dfc20829a7429f72022571558093e 06c3da95c71d253a3244f2e194c38618ad64f98e -- crates/fff-core/src/score.rs
printf '%s\n' '--- comparator and pagination implementation ---'
sed -n '1090,1170p' crates/fff-core/src/score.rs | nl -ba -v1090
printf '%s\n' '--- pagination test ---'
sed -n '1490,1570p' crates/fff-core/src/score.rs | nl -ba -v1490
printf '%s\n' '--- FileItem declaration and ordering/key helpers ---'
rg -n -C 8 'struct FileItem|impl FileItem|fn .*path|relative_path|file_name\\(' crates/fff-core/src/file_picker.rs crates/fff-core/src/score.rs
printf '%s\n' '--- lifecycle source ---'
sed -n '610,675p' crates/fff-core/src/watcher/background_watcher.rs | nl -ba -v610
sed -n '1635,1670p' crates/fff-core/src/file_picker.rs | nl -ba -v1635
sed -n '1735,1775p' crates/fff-core/src/file_picker.rs | nl -ba -v1735
sed -n '2140,2210p' crates/fff-core/src/file_picker.rs | nl -ba -v2140
printf '%s\n' '--- pagination contract and consumers ---'
rg -n -i 'pagination|page_index|page_size|deterministic.*(page|result)|stable.*(page|result)' README.md crates docs 2>/dev/null | head -160Repository: dmtrKovalenko/fff Length of output: 12522 🤖 get_repo_knowledge executed:
Length of output: 44178 🏁 Script executed: #!/bin/bash
printf '%s\n' '--- FileItem and path identity declarations ---'
rg -n -C 10 'pub struct FileItem|struct FileItem|impl FileItem|relative_path_eq|file_name\(' crates/fff-core/src/types.rs crates/fff-core/src/file_picker.rs crates/fff-core/src/score.rs
printf '%s\n' '--- watcher append and rescan path ---'
rg -n -C 7 'handle_create_or_modify|add_new_file|files\.push|trigger_full_rescan_async|commit_new_sync|paths_to_add_or_modify' crates/fff-core/src/watcher/background_watcher.rs crates/fff-core/src/file_picker.rs crates/fff-core/src/shared.rs
printf '%s\n' '--- scan ordering and rebuilt index ---'
rg -n -C 8 'par_sort_unstable_by|base_count|from_vec_with_reserve|walk_filesystem' crates/fff-core/src/file_picker.rs crates/fff-core/src/scan.rs
printf '%s\n' '--- repository pagination/determinism contracts ---'
git grep -n -i -E 'pagination|deterministic.*(page|result)|stable.*(page|result)' 06c3da95c71d253a3244f2e194c38618ad64f98e -- README.md docs crates/fff-core crates/fff-nvim lua 2>/dev/null | head -160Repository: dmtrKovalenko/fff Length of output: 41861 Tie by file identity, not address. When a watcher appends a matching file and a full rescan rebuilds the index, the same logical files can occupy different slots. This comparator orders equal-score, equal-mtime items by 🤖 Prompt for AI Agents |
||
| }; | ||
| // Use partial sort if we need less than half the results and dataset is large | ||
| if items_needed < total_matched / 2 && total_matched > 100 { | ||
|
|
@@ -1515,6 +1519,44 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn tied_scores_paginate_deterministically() { | ||
| let files: Vec<_> = (0..3000) | ||
| .map(|_| FileItem::new_raw(0, 0, 1, None, false)) | ||
| .collect(); | ||
| let parser = QueryParser::default(); | ||
| let query = parser.parse(""); | ||
| let page = |shuffle: usize, offset: usize| { | ||
| let mut results: Vec<_> = files.iter().map(|file| (file, Score::default())).collect(); | ||
| results.rotate_left(shuffle); | ||
| results.reverse(); | ||
| let context = ScoringContext { | ||
| query: &query, | ||
| max_threads: 1, | ||
| max_typos: 0, | ||
| project_path: None, | ||
| current_file: None, | ||
| last_same_query_match: None, | ||
| combo_boost_score_multiplier: 0, | ||
| min_combo_count: 0, | ||
| pagination: PaginationArgs { offset, limit: 8 }, | ||
| }; | ||
| sort_and_paginate(results, &context, |s| s.total).0 | ||
| }; | ||
| for offset in [0, 8, 16] { | ||
| let expected: Vec<_> = files[offset..offset + 8].iter().collect(); | ||
| for shuffle in [0, 7, 1500, 2999] { | ||
| assert!( | ||
| page(shuffle, offset) | ||
| .iter() | ||
| .zip(&expected) | ||
| .all(|(a, b)| std::ptr::eq(*a, *b)), | ||
| "offset={offset} shuffle={shuffle}" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn merge_offsets_reuses_storage() { | ||
| let mut ranges: SmallVec<[(u32, u32); 4]> = smallvec::smallvec![ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: dmtrKovalenko/fff
Length of output: 41627
🏁 Script executed:
Repository: dmtrKovalenko/fff
Length of output: 22185
🏁 Script executed:
Repository: dmtrKovalenko/fff
Length of output: 41903
🏁 Script executed:
Repository: dmtrKovalenko/fff
Length of output: 5554
🏁 Script executed:
Repository: dmtrKovalenko/fff
Length of output: 25618
🏁 Script executed:
Repository: dmtrKovalenko/fff
Length of output: 29385
Use a stable path tie-break for directory results. Watcher additions append new directories in event order, while a rescan rebuilds the directory table in sorted path order.
StableVecpointer order follows slots, so when the same directories remain tied and the offset cuts through that group, the same query can return a different page. Compare relative paths instead.🤖 Prompt for AI Agents