Fix librarian work title sorting not ignoring articles#11463
Fix librarian work title sorting not ignoring articles#11463
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes librarian work title and author name sorting by migrating from Solr-based text processing to Python preprocessing combined with ICU collation fields.
- Implements title_sort preprocessing in Python using the same article removal regex previously in Solr
- Replaces
name_strwithname_sortusing ICUCollationField for proper author name sorting - Updates Solr configuration from 9.4 to 9.12 and standardizes analysis-extras module configuration across environments
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openlibrary/solr/updater/work.py | Adds ARTICLE_PATTERN regex and title_sort property to process titles before Solr indexing |
| openlibrary/solr/types_generator.py | Renames text_title_sort to text_international_sort type mapping |
| openlibrary/solr/solr_types.py | Replaces name_str with name_sort in SolrDocument type definition |
| openlibrary/plugins/worksearch/schemes/authors.py | Updates author sorting to use name_sort instead of name_str |
| conf/solr/conf/solrconfig.xml | Updates luceneMatchVersion to 9.12, incorporates Solr config improvements, fixes grammar in comment |
| conf/solr/conf/managed-schema.xml | Replaces text_title_sort with text_international_sort ICUCollationField, updates copyFields, fixes typo |
| compose.yaml | Standardizes Solr module configuration using -Dsolr.modules instead of SOLR_MODULES |
| compose.production.yaml | Adds missing analysis-extras module configuration |
| compose.override.yaml | Adds missing analysis-extras module configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'key': wkey.replace("/books/", "/works/"), | ||
| 'type': {'key': '/type/work'}, | ||
| 'title': work.get('title'), | ||
| 'title_sort': work.get('title_sort'), |
There was a problem hiding this comment.
The title_sort field is not available in edition records. This line tries to get title_sort from an edition dict when work['type']['key'] == '/type/edition', but editions don't have a title_sort field - only the WorkSolrBuilder class has a title_sort property. This will always be None for editions and won't properly sort edition titles. Consider computing title_sort here using the same logic as the title_sort property (lines 332-350), or extracting that logic into a helper function.
|
@mekarpeles @cdrini Is there a problem with this PR? If so, can you outline what needs changing? What do I need to do to get it reviewed and merged? It's been open a month and no one has even looked at it yet, which doesn't encourage me to invest in the followup work that I had intended to fix other search problems. |
|
Howdy @tfmorris , apologies for the delay. I unfortunately have a rather large backlog of prs, and have been pulled away fighting a lot of our performance issues as a result of abusive traffic. I'll try to get a code review for you next week 👍 I will note, that smaller, single issue/purpose PRs are the best way to help us get through PRs quickly; they're easier to test and lower risk, so they move through much faster. |
|
Thanks for the reply.
I'm happy to split this up if you provide guidance on how you'd like it divided. There were two bugs introduced in PR #11211 which this fixes. Issue #11430 opened by @mheiman documents one of the two bugs, but I chose to fix both together, since they were both introduced in the same PR. Ironically, that PR got reviewed, approved, and "auto-merged" in under 24 hours. Let me know how you'd like it split up. All the changes are in separate commits, so it's easy enough to turn this into 6 separate PRs. |
|
@mekarpeles @cdrini This PR has been open 6 months now without review. I am desperately seeking feedback on how to resolve this. What do I need to do to move it forward? |
There was a problem hiding this comment.
Hi @tfmorris apologies, I know the conflicts are frustrating. If you make this PR tackle only the problem stated in the issue 11430, that will make this possible for me to review. Changing other fields makes these take much longer to test and balloons the scope of risk. The solr 10 PR update should hopefully simplify this PR and make it easier to merge, since it should remove the largest changes. It will also let us test the new solr defaults around docValues independently of the changes related to the title sorting.
But regardless, apologies again for the long delay. I've tried a few times to get through this one but it takes too long for me to review and test. The solr 10 changes alone, which are essentially a subset of the changes you've made here, took hours to test and fix. I should have given you the above feedback much earlier so that you could act on it. That's on me.
I've attached the comments from the last time I tried to review this but ran out of time; so note it's incomplete. Shrinkwraping the changes to the problem described in the issue and I'll be able to review and merge. Also providing clear instructions for how you tested this throughout the site, and specifically how you tested that this addresses the issue, make it faster.
|
Splitting this up is easy to do. The different sets of changes are already isolated into different commits. I'll create separate PRs with the individual commits.
That basically replicated the work in 4db39c3, so it's easy to drop that commit. |
- Use our new ICU international collating order which should perform reasonably across a variety of languages without being language-specific. Because it only stores a collation key, storage usage is minimal. - Move article (Le, La, Les, The, An, ...) processing to the Solr updater since the ICUCollationFilter doesn't support an analyze pipeline. - Remove the unused text_title_sort type
d3878fa to
ff4183f
Compare
|
@cdrini I've added sort titles for editions and removed everything not directly related to work & edition sort titles. |
|
@cdrini The merge conflicts from the merge of the other Solr PR have been fixed and this is ready for review. |
There was a problem hiding this comment.
Nice looks great! Note as-is it wasn't loading correctly because the old copyField apparently needs to be deleted. When I spun this up locally, the update commands were silently failing (although I couldn't track down the error). Removing the copyField fixed it though.
Tested bringing things up works, and locally sorting now ignores eg "The".
There was a problem hiding this comment.
We'll need to remove the copyField; it appears to silently fail when trying to write to it directly; removing the copyField fixes it.
But this does make the migration path a bit tricky. I can remove the copyField's from our production solrs, but that leaves a window where newly edited records won't have this field. And then patch deploy this out to solr-updater so it starts writing things. I think that should do the trick. Marking as "Needs: Special Deploy"
| return re_not_az.sub("", pub).lower() == "sn" | ||
|
|
||
|
|
||
| ARTICLE_PATTERN = re.compile( |
There was a problem hiding this comment.
Moving this to the python is great ; in a future issue/PR, we should update this to be language-aware, and use different articles for different languages!
Co-authored-by: GitHub Copilot <github-copilot[bot]@users.noreply.github.com>
| return self._edition.get("subtitle") | ||
|
|
||
| @property | ||
| def title_sort(self) -> str | None: |
There was a problem hiding this comment.
Note this won't add it to the edition ; we'd need to specify it on the dict in the build function. Note sure if that was intentional or not. We are usually rather wary of adding fields to the edition-level documents for perf since there are a lot of editions, and since we don't have a strategy for how we would use that in order to allow our edition-aware search to sort by edition title, I think that might make sense as a future issues/PR.
|
Ok, I:
|
Closes #11430. Reverses damage from #11211.
Modifies #11188.fix
Technical
Fixes:
2. Uses the same type of collation field for author names, but without any special pre-processingSplit to Fix author search name sort #12809Related changes:1. Incorporated diffs from Solr 9.4->9.9 intosolrconfig.xmlso that OL config is based on the current default config. Modifies what was done in #111882. Made configuration of analysis-extras consistent across all environments. Also refs #11188.
3. Removed unused fields
name_strandtitle_suggest. The latter saves 3.4 GB for the current 55 M editions and the former eliminates the duplicate name storage for 15 M authors saving another 300+ MB.Because the title pre-processing is now done in Python where more context is available, a potential future enhancement would be to only remove articles for the language that the title is in, rather than using a generic multi-lingual regex which will have both false positives and false negatives. Note that language specific collation is likely not an option since it would depend on all titles being in the same language.
Testing
Use standard Solr post-reindexing test suite.
Stakeholders
@mheiman