Conversation
LCM Tests 8 files - 8 8 suites - 8 1m 4s ⏱️ - 1m 2s Results for commit 3296b5c. ± Comparison against base commit 9fdb060. This pull request skips 23 tests.♻️ This comment has been updated with latest results. |
Add a Substring value to SearchType that matches the query anywhere within a string, case- and diacritic-insensitive, backed by a raw-string index scanned with CompareInfo.IndexOf. The existing Exact/Prefix/FullText modes are unchanged.
| return Enumerable.Empty<T>(); | ||
| CompareInfo ci = CultureInfo.InvariantCulture.CompareInfo; | ||
| return raw.Where(kv => ci.IndexOf(kv.Value, text, | ||
| CompareOptions.IgnoreCase | CompareOptions.IgnoreNonSpace) >= 0).Select(kv => kv.Key); |
There was a problem hiding this comment.
we might need to consider if this will work correctly for us, or if we need to create a manged wrapper in icu-dotnet for string search so we can use what ICU provides for this use case.
There was a problem hiding this comment.
It seems that this is the same behavior as FWLite's search, so hopefully it will be a good way to go?
I did find another behavior to include, see the "Fold diacritics only when the search term has none" change here.
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
This looks good. I left some suggestions, nothing blocking.
One thing, since the issue specifically calls out matching how FW Lite search works, here's some of our tests which would be relevant here
https://github.com/sillsdev/languageforge-lexbox/blob/bbcc058f267a789302b9570e9542d30968199a3f/backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs#L412-L455
Up to you how closely you try to match what FW Lite does.
|
Thank you! I'm looking into getting some of the enhancements you suggested in. |
- Build the sort-key index only in the search types that use it, so Substring no longer creates and discards an unused index (in both Add and Search). - Skip indexing null or empty text in Add instead of coercing it to an empty string. - Replace KeyValuePair<T, string> in the raw index with a named SubstringEntry struct for readability. Co-Authored-By: Claude Opus <noreply@anthropic.com>
- Fold diacritics for substring only when the query itself has none, so an unmarked query matches accented text but an accented query is specific. Added tests inspired by FWLite mirroring its SuccessfulMatches/NegativeMatches and NFC/NFD cases. Co-Authored-By: Claude Opus <noreply@anthropic.com>
thejambi
left a comment
thejambi
left a comment
There was a problem hiding this comment.
@thejambi made 3 comments.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on hahn-kev).
| return Enumerable.Empty<T>(); | ||
| CompareInfo ci = CultureInfo.InvariantCulture.CompareInfo; | ||
| return raw.Where(kv => ci.IndexOf(kv.Value, text, | ||
| CompareOptions.IgnoreCase | CompareOptions.IgnoreNonSpace) >= 0).Select(kv => kv.Key); |
There was a problem hiding this comment.
It seems that this is the same behavior as FWLite's search, so hopefully it will be a good way to go?
I did find another behavior to include, see the "Fold diacritics only when the search term has none" change here.
| } | ||
|
|
||
| public T Item { get { return m_item; } } | ||
| public string Text { get { return m_text; } } |
There was a problem hiding this comment.
you could simplify this to public string Text => m_text;
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
Nice! Looks great, I just left one minor suggestion, feel free to ignore it.
2 participants
Add a Substring value to SearchType that matches the query anywhere within a string, case- and diacritic-insensitive, backed by a raw-string index scanned with CompareInfo.IndexOf. The existing Exact/Prefix/FullText modes are unchanged. This change enables a change in FieldWorks to use this new SearchType, see sillsdev/FieldWorks#1069
https://jira.sil.org/browse/LT-22524
This change is