mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Prioritize filename matches in fuzzy file search (#59709)
## Why Matches in parent directory names can crowd matching filenames out of limited search results. For example, searching for `ViewModel` should keep `ChatViewModel.kt` visible even when many directories are named `viewmodel`. ## What changed - Rank basename matches before applying the result limit for single positive fuzzy queries without path separators. - Prefer exact filenames, then the strongest exact directory match, then other matching filenames. Leave additional directory matches ranked by their paths. - Compute highlight indices from the basename for promoted results and return scores that reflect the new ranking. - Rank snapshots in resumable time slices so new queries and cancellation can interrupt the work. ## Testing Add regression coverage for filename matches surviving the result limit, exact file and directory priority, path queries, and resuming partial rankings. Update app-server score expectations while preserving highlight indices. GitOrigin-RevId: 41b3420581b4f6490859e809245370b737889544
This commit is contained in:
committed by
copyberry
parent
054128c9d9
commit
0327a3ec34
@@ -262,7 +262,8 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> {
|
||||
.await??;
|
||||
|
||||
let value = resp.result;
|
||||
let expected_score = 72;
|
||||
// Each basename matches, so its score includes one filename priority bonus.
|
||||
let filename_bonus = u32::from(u16::MAX) + 1;
|
||||
|
||||
assert_eq!(
|
||||
value,
|
||||
@@ -273,7 +274,7 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> {
|
||||
"path": "abexy",
|
||||
"match_type": "file",
|
||||
"file_name": "abexy",
|
||||
"score": 84,
|
||||
"score": filename_bonus + 84,
|
||||
"indices": [0, 1, 2],
|
||||
},
|
||||
{
|
||||
@@ -281,7 +282,7 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> {
|
||||
"path": sub_abce_rel,
|
||||
"match_type": "file",
|
||||
"file_name": "abce",
|
||||
"score": expected_score,
|
||||
"score": filename_bonus + 72,
|
||||
"indices": [4, 5, 7],
|
||||
},
|
||||
{
|
||||
@@ -289,7 +290,7 @@ async fn test_fuzzy_file_search_sorts_and_includes_indices() -> Result<()> {
|
||||
"path": "abcde",
|
||||
"match_type": "file",
|
||||
"file_name": "abcde",
|
||||
"score": 71,
|
||||
"score": filename_bonus + 71,
|
||||
"indices": [0, 1, 4],
|
||||
},
|
||||
]
|
||||
|
||||
@@ -41,7 +41,7 @@ pub use cli::Cli;
|
||||
|
||||
/// A single match result returned from the search.
|
||||
///
|
||||
/// * `score` – Relevance score returned by `nucleo`.
|
||||
/// * `score` – Relevance score used to select and order this result.
|
||||
/// * `path` – Path to the matched entry (file or directory), relative to the
|
||||
/// search directory.
|
||||
/// * `match_type` – Whether this match is a file or directory.
|
||||
@@ -520,7 +520,7 @@ fn matcher_worker(
|
||||
) -> anyhow::Result<()> {
|
||||
const TICK_TIMEOUT_MS: u64 = 10;
|
||||
let config = Config::DEFAULT.match_paths();
|
||||
let mut indices_matcher = inner.compute_indices.then(|| Matcher::new(config.clone()));
|
||||
let mut matcher = Matcher::new(config);
|
||||
let cancel_requested = || inner.cancelled.load(Ordering::Relaxed);
|
||||
let shutdown_requested = || inner.shutdown.load(Ordering::Relaxed);
|
||||
|
||||
@@ -528,6 +528,7 @@ fn matcher_worker(
|
||||
let mut next_notify = never();
|
||||
let mut will_notify = false;
|
||||
let mut walk_complete = false;
|
||||
let mut ranking = None;
|
||||
|
||||
loop {
|
||||
select! {
|
||||
@@ -537,6 +538,7 @@ fn matcher_worker(
|
||||
};
|
||||
match signal {
|
||||
WorkSignal::QueryUpdated(query) => {
|
||||
ranking = None;
|
||||
let append = query.starts_with(&last_query);
|
||||
nucleo.pattern.reparse(
|
||||
0,
|
||||
@@ -569,20 +571,26 @@ fn matcher_worker(
|
||||
}
|
||||
recv(next_notify) -> _ => {
|
||||
will_notify = false;
|
||||
let status = nucleo.tick(TICK_TIMEOUT_MS);
|
||||
if status.changed
|
||||
&& let Some(snapshot) = snapshot::for_query(
|
||||
&inner,
|
||||
&nucleo,
|
||||
&last_query,
|
||||
&mut indices_matcher,
|
||||
walk_complete,
|
||||
)
|
||||
{
|
||||
inner.reporter.on_update(&snapshot);
|
||||
if ranking.is_none() {
|
||||
let status = nucleo.tick(TICK_TIMEOUT_MS);
|
||||
if status.changed {
|
||||
ranking = snapshot::Ranking::start(&nucleo, &last_query, walk_complete);
|
||||
}
|
||||
if ranking.is_none() && !status.running && walk_complete {
|
||||
inner.reporter.on_complete();
|
||||
}
|
||||
}
|
||||
if !status.running && walk_complete {
|
||||
inner.reporter.on_complete();
|
||||
if let Some(active) = &mut ranking {
|
||||
if let Some(snapshot) = active.advance(
|
||||
&inner, &nucleo, &last_query, &mut matcher,
|
||||
Duration::from_millis(TICK_TIMEOUT_MS),
|
||||
) {
|
||||
inner.reporter.on_update(&snapshot);
|
||||
ranking = None;
|
||||
}
|
||||
// Resume this snapshot or collect work that arrived while ranking it.
|
||||
will_notify = true;
|
||||
next_notify = after(Duration::ZERO);
|
||||
}
|
||||
}
|
||||
default(Duration::from_millis(100)) => {
|
||||
|
||||
@@ -47,6 +47,50 @@ fn tie_breakers_sort_by_path_when_scores_equal() {
|
||||
assert_eq!(matches, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn matching_filenames_survive_the_path_result_limit() {
|
||||
let root = tempfile::tempdir().unwrap();
|
||||
let folder = root.path().join("viewmodel");
|
||||
fs::create_dir(&folder).unwrap();
|
||||
fs::write(folder.join("build.gradle.kts"), "").unwrap();
|
||||
let source = Path::new("feature").join("src");
|
||||
fs::create_dir_all(root.path().join(&source)).unwrap();
|
||||
let filename = source.join("ChatViewModel.kt");
|
||||
fs::write(root.path().join(&filename), "").unwrap();
|
||||
|
||||
let search = |query, limit| {
|
||||
run(
|
||||
query,
|
||||
vec![root.path().to_path_buf()],
|
||||
FileSearchOptions {
|
||||
limit: NonZero::new(limit).unwrap(),
|
||||
..Default::default()
|
||||
},
|
||||
/*cancel_flag*/ None,
|
||||
)
|
||||
.unwrap()
|
||||
.matches
|
||||
};
|
||||
for i in 0..80 {
|
||||
fs::create_dir_all(root.path().join(format!("parent-{i}/viewmodel"))).unwrap();
|
||||
}
|
||||
let hits = search("ViewModel", 2);
|
||||
assert_eq!(hits[0].match_type, MatchType::Directory);
|
||||
assert_eq!(hits[1].path, filename);
|
||||
assert_eq!(search("VM", 1)[0].path, filename);
|
||||
fs::create_dir(root.path().join("src")).unwrap();
|
||||
for i in 0..80 {
|
||||
fs::write(root.path().join(format!("source-{i}.rs")), "").unwrap();
|
||||
}
|
||||
assert_eq!(search("src", 1)[0].path, Path::new("src"));
|
||||
fs::write(root.path().join(&source).join("src"), "").unwrap();
|
||||
assert_eq!(search("src", 1)[0].path, source.join("src"));
|
||||
assert_eq!(
|
||||
search(&format!("viewmodel{}", std::path::MAIN_SEPARATOR), 1)[0].path,
|
||||
Path::new("viewmodel").join("build.gradle.kts")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_name_from_path_uses_basename() {
|
||||
assert_eq!(file_name_from_path("foo/bar.txt"), "bar.txt");
|
||||
|
||||
@@ -40,15 +40,17 @@ fn query_update_does_not_relabel_pending_matches() {
|
||||
reporter: Arc::new(tests::RecordingReporter::default()),
|
||||
work_tx,
|
||||
};
|
||||
let mut indices_matcher = Some(Matcher::new(Config::DEFAULT.match_paths()));
|
||||
let initial = snapshot::for_query(
|
||||
&inner,
|
||||
&nucleo,
|
||||
"",
|
||||
&mut indices_matcher,
|
||||
/*walk_complete*/ true,
|
||||
)
|
||||
.unwrap();
|
||||
let mut matcher = Matcher::new(Config::DEFAULT.match_paths());
|
||||
let snapshot = |nucleo: &Nucleo<IndexedEntry>, query: &str, matcher: &mut Matcher| {
|
||||
snapshot::Ranking::start(nucleo, query, /*walk_complete*/ true)?.advance(
|
||||
&inner,
|
||||
nucleo,
|
||||
query,
|
||||
matcher,
|
||||
Duration::from_secs(5),
|
||||
)
|
||||
};
|
||||
let initial = snapshot(&nucleo, "", &mut matcher).unwrap();
|
||||
assert_eq!(initial.matches.len(), 2);
|
||||
|
||||
nucleo.pattern.reparse(
|
||||
@@ -60,32 +62,17 @@ fn query_update_does_not_relabel_pending_matches() {
|
||||
);
|
||||
// The requested pattern has changed, but no tick has replaced the previous
|
||||
// snapshot. This is the same intermediate state a busy tick can expose.
|
||||
assert_eq!(
|
||||
snapshot::for_query(
|
||||
&inner,
|
||||
&nucleo,
|
||||
"bet",
|
||||
&mut indices_matcher,
|
||||
/*walk_complete*/ true
|
||||
),
|
||||
None
|
||||
);
|
||||
assert_eq!(snapshot(&nucleo, "bet", &mut matcher), None);
|
||||
|
||||
while nucleo.tick(/*timeout*/ 10).running {
|
||||
work_rx.recv_timeout(Duration::from_secs(5)).unwrap();
|
||||
}
|
||||
assert_eq!(
|
||||
snapshot::for_query(
|
||||
&inner,
|
||||
&nucleo,
|
||||
"bet",
|
||||
&mut indices_matcher,
|
||||
/*walk_complete*/ true
|
||||
),
|
||||
snapshot(&nucleo, "bet", &mut matcher),
|
||||
Some(FileSearchSnapshot {
|
||||
query: "bet".to_string(),
|
||||
matches: vec![FileMatch {
|
||||
score: 84,
|
||||
score: 65_536 + 84,
|
||||
path: PathBuf::from("beta.txt"),
|
||||
match_type: MatchType::File,
|
||||
root: root.path().to_path_buf(),
|
||||
@@ -96,4 +83,27 @@ fn query_update_does_not_relabel_pending_matches() {
|
||||
walk_complete: true,
|
||||
}),
|
||||
);
|
||||
// Force the pending ranking to span worker turns, then resume it on the same snapshot.
|
||||
for i in 0..256 {
|
||||
injector.push(
|
||||
IndexedEntry::Ascii {
|
||||
root_idx: 0,
|
||||
match_type: MatchType::File,
|
||||
},
|
||||
|_, columns| columns[0] = Utf32String::from(format!("bet/path-{i}")),
|
||||
);
|
||||
}
|
||||
while nucleo.tick(/*timeout*/ 10).running {
|
||||
work_rx.recv_timeout(Duration::from_secs(5)).unwrap();
|
||||
}
|
||||
let mut partial = snapshot::Ranking::start(&nucleo, "bet", /*walk_complete*/ true).unwrap();
|
||||
assert!(
|
||||
partial
|
||||
.advance(&inner, &nucleo, "bet", &mut matcher, Duration::ZERO)
|
||||
.is_none()
|
||||
);
|
||||
assert_eq!(
|
||||
partial.advance(&inner, &nucleo, "bet", &mut matcher, Duration::from_secs(5)),
|
||||
snapshot(&nucleo, "bet", &mut matcher)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1,34 +1,162 @@
|
||||
//! Converts matcher snapshots without associating old results with a new query.
|
||||
//! Ranks a stable matcher snapshot in slices so the worker can handle new queries.
|
||||
|
||||
use crate::FileMatch;
|
||||
use crate::FileSearchSnapshot;
|
||||
use crate::IndexedEntry;
|
||||
use crate::MatchType;
|
||||
use crate::SessionInner;
|
||||
use nucleo::Match;
|
||||
use nucleo::Matcher;
|
||||
use nucleo::Nucleo;
|
||||
use nucleo::Utf32Str;
|
||||
use nucleo::Utf32String;
|
||||
use nucleo::pattern::AtomKind;
|
||||
use std::cmp::Reverse;
|
||||
use std::collections::BinaryHeap;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
pub(super) fn for_query(
|
||||
// A single Nucleo atom returns a u16. Leave room for its full score within each group.
|
||||
const NAME_BONUS: u32 = u16::MAX as u32 + 1;
|
||||
|
||||
#[derive(Default)]
|
||||
pub(super) struct Ranking {
|
||||
next: usize,
|
||||
best: BinaryHeap<(Reverse<u32>, usize)>,
|
||||
found_exact_folder: bool,
|
||||
rank_names: bool,
|
||||
walk_complete: bool,
|
||||
}
|
||||
|
||||
impl Ranking {
|
||||
pub(super) fn start(
|
||||
nucleo: &Nucleo<IndexedEntry>,
|
||||
query: &str,
|
||||
walk_complete: bool,
|
||||
) -> Option<Self> {
|
||||
let pattern = nucleo.snapshot().pattern().column_pattern(0);
|
||||
// A tick can collect the previous query while the new one is running.
|
||||
if pattern.atoms != nucleo.pattern.column_pattern(0).atoms {
|
||||
return None;
|
||||
}
|
||||
let rank_names = matches!(pattern.atoms.as_slice(), [atom] if atom.kind == AtomKind::Fuzzy && !atom.negative)
|
||||
&& !query.contains(['/', '\\']);
|
||||
Some(Self {
|
||||
rank_names,
|
||||
walk_complete,
|
||||
..Self::default()
|
||||
})
|
||||
}
|
||||
|
||||
/// Yields to the worker between slices so a new query or cancellation can preempt this snapshot.
|
||||
pub(super) fn advance(
|
||||
&mut self,
|
||||
inner: &SessionInner,
|
||||
nucleo: &Nucleo<IndexedEntry>,
|
||||
query: &str,
|
||||
matcher: &mut Matcher,
|
||||
budget: Duration,
|
||||
) -> Option<FileSearchSnapshot> {
|
||||
let snapshot = nucleo.snapshot();
|
||||
let ranked = snapshot.matches();
|
||||
if !self.rank_names {
|
||||
return Some(collect(
|
||||
inner,
|
||||
nucleo,
|
||||
query,
|
||||
matcher,
|
||||
self.walk_complete,
|
||||
ranked
|
||||
.iter()
|
||||
.copied()
|
||||
.take(inner.limit)
|
||||
.map(|matched| (matched, false)),
|
||||
));
|
||||
}
|
||||
let name = &snapshot.pattern().column_pattern(0).atoms[0];
|
||||
let deadline = Instant::now() + budget;
|
||||
for (matched, item) in ranked[self.next..]
|
||||
.iter()
|
||||
.zip(snapshot.matched_items(self.next as u32..))
|
||||
{
|
||||
let kind = match item.data {
|
||||
IndexedEntry::Ascii { match_type, .. } => *match_type,
|
||||
IndexedEntry::Unicode(entry) => entry.match_type,
|
||||
};
|
||||
let basename = basename(item.matcher_columns[0].slice(..));
|
||||
let exact_length = basename.len() == name.needle_text().len();
|
||||
let score = match kind {
|
||||
MatchType::File => name.score(basename, matcher).map(|score| {
|
||||
let bonus = if exact_length {
|
||||
3 * NAME_BONUS
|
||||
} else {
|
||||
NAME_BONUS
|
||||
};
|
||||
bonus + u32::from(score)
|
||||
}),
|
||||
// Keep the strongest exact folder. More folders with the same name
|
||||
// compete in path order so they cannot crowd out matching files.
|
||||
MatchType::Directory
|
||||
if !self.found_exact_folder
|
||||
&& exact_length
|
||||
&& name.score(basename, matcher).is_some() =>
|
||||
{
|
||||
self.found_exact_folder = true;
|
||||
Some(2 * NAME_BONUS + matched.score)
|
||||
}
|
||||
MatchType::Directory => None,
|
||||
}
|
||||
.unwrap_or(matched.score);
|
||||
// The heap head is the lowest score, then the latest path on a tie.
|
||||
let candidate = (Reverse(score), self.next);
|
||||
if self.best.len() < inner.limit {
|
||||
self.best.push(candidate);
|
||||
} else if let Some(mut worst) = self.best.peek_mut()
|
||||
&& candidate < *worst
|
||||
{
|
||||
*worst = candidate;
|
||||
}
|
||||
self.next += 1;
|
||||
if self.next.is_multiple_of(64) && Instant::now() >= deadline {
|
||||
return None;
|
||||
}
|
||||
}
|
||||
let selected = std::mem::take(&mut self.best)
|
||||
.into_sorted_vec()
|
||||
.into_iter()
|
||||
.map(|(Reverse(score), index)| {
|
||||
(
|
||||
Match {
|
||||
score,
|
||||
idx: ranked[index].idx,
|
||||
},
|
||||
score >= NAME_BONUS,
|
||||
)
|
||||
});
|
||||
Some(collect(
|
||||
inner,
|
||||
nucleo,
|
||||
query,
|
||||
matcher,
|
||||
self.walk_complete,
|
||||
selected,
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
fn collect(
|
||||
inner: &SessionInner,
|
||||
nucleo: &Nucleo<IndexedEntry>,
|
||||
query: &str,
|
||||
indices_matcher: &mut Option<Matcher>,
|
||||
matcher: &mut Matcher,
|
||||
walk_complete: bool,
|
||||
) -> Option<FileSearchSnapshot> {
|
||||
candidates: impl Iterator<Item = (Match, bool)>,
|
||||
) -> FileSearchSnapshot {
|
||||
let snapshot = nucleo.snapshot();
|
||||
let pattern = snapshot.pattern().column_pattern(0);
|
||||
// A tick can collect the previous query's results while the new query is
|
||||
// still running. Wait for a matching snapshot before using the new label.
|
||||
if pattern.atoms != nucleo.pattern.column_pattern(0).atoms {
|
||||
return None;
|
||||
}
|
||||
let limit = inner.limit.min(snapshot.matched_item_count() as usize);
|
||||
let matches = snapshot
|
||||
.matches()
|
||||
.iter()
|
||||
.take(limit)
|
||||
.filter_map(|match_| {
|
||||
let matches = candidates
|
||||
.filter_map(|(match_, matched_name)| {
|
||||
let item = snapshot.get_item(match_.idx)?;
|
||||
let (root_idx, relative_path, match_type) = match item.data {
|
||||
IndexedEntry::Ascii {
|
||||
@@ -46,10 +174,14 @@ pub(super) fn for_query(
|
||||
entry.match_type,
|
||||
),
|
||||
};
|
||||
let indices = if let Some(indices_matcher) = indices_matcher.as_mut() {
|
||||
let indices = if inner.compute_indices {
|
||||
let mut indices = Vec::<u32>::new();
|
||||
let haystack = item.matcher_columns[0].slice(..);
|
||||
let _ = pattern.indices(haystack, indices_matcher, &mut indices);
|
||||
let path = item.matcher_columns[0].slice(..);
|
||||
let haystack = if matched_name { basename(path) } else { path };
|
||||
let _ = pattern.indices(haystack, matcher, &mut indices);
|
||||
for index in &mut indices {
|
||||
*index += (path.len() - haystack.len()) as u32;
|
||||
}
|
||||
indices.sort_unstable();
|
||||
indices.dedup();
|
||||
Some(indices)
|
||||
@@ -65,11 +197,20 @@ pub(super) fn for_query(
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
Some(FileSearchSnapshot {
|
||||
FileSearchSnapshot {
|
||||
query: query.to_string(),
|
||||
matches,
|
||||
total_match_count: snapshot.matched_item_count() as usize,
|
||||
scanned_file_count: snapshot.item_count() as usize,
|
||||
walk_complete,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
fn basename(path: Utf32Str<'_>) -> Utf32Str<'_> {
|
||||
let len = path
|
||||
.chars()
|
||||
.rev()
|
||||
.take_while(|c| !std::path::is_separator(*c))
|
||||
.count();
|
||||
path.slice(path.len() - len..)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user