|
Description
David Gustafsson
2020-03-05 09:40:55 UTC
Created attachment 100170 [details] [review] Bug 24807: Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowing out relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Created attachment 100171 [details] [review] Bug 24807: Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowning out more relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Created attachment 100176 [details] [review] Bug 24807: Add database update script Created attachment 102547 [details] [review] Bug 24807: Update tests Created attachment 102548 [details] [review] Bug 24807: Add suppport for uncertain fields and ranges To test: 1 - Have some records with uncertain dates in the 008 19uu, 195u, etc. 2 - Index them in Elasticsearch 3 - Do a search that will return them 4 - Sort results by publication/copyright date 5 - Note odd results 6 - Apply patch 7 - Reindex 8 - Sorting should be improved Hi David, I like your patch better than mine (bug 24559) but I do think we should support uncertain dates and ranges in these fields as they do meet MARC standards. 195u isn't exactly 1950, but if looking for a range from 1940-1960 I would expect it to return so I think indexing as 1950 is reasonable In testing your code did not work with dates like c1964 I added tests and made some adjustment. The array stuff may need work, depending on if we would support being a year field and having non-filing characters, but I can't see a reason to do that. Please let me know what you think of these patches. -Nick Hi! Sorry about the late reply. Thank you very much for considering my patch. I am a little bit unfamiliar with conventions with regards to marc record dates and did not anything like c1964 in my testing data, is "c" in this case for copyright? Is "u" some kind of convention that should have a special case? I think there is another callback option "value_callbacks" that can be used for preprocessing values before filtering, for example replace "u" with "0", and strip other characters. I can update the patch using this approach later today, I think it is a little bit semantically to perform filtering and transformation in different steps. I get: error: sha1 information is lacking or useless (t/Koha/SearchEngine/Elasticsearch.t). error: could not build fake ancestor Patch failed at 0001 Bug 24807: Update tests When applying the patch. I think the first two commits needs to be rebased since they do not apply anymore. The last two seems to be using a different base. I might be able to work this out, but will have to wait for tomorrow. (In reply to David Gustafsson from comment #7) > Hi! Sorry about the late reply. Thank you very much for considering my > patch. I am a little bit unfamiliar with conventions with regards to marc > record dates and did not anything like c1964 in my testing data, is "c" in > this case for copyright? Is "u" some kind of convention that should have a > special case? I think there is another callback option "value_callbacks" > that can be used for preprocessing values before filtering, for example > replace "u" with "0", and strip other characters. I can update the patch > using this approach later today, I think it is a little bit semantically to > perform filtering and transformation in different steps. There are more fields than just the 008 that can contain 'year' - pubdate and copydate are the main two used in koha 008/264c/260c The u is unknown, it is documented in the MARC: https://www.loc.gov/marc/bibliographic/bd008a.html search for (195u) The c is copyright we can also have p or ranges etc., documented here: https://www.oclc.org/bibformats/en/2xx/264.html#subfieldc Created attachment 103084 [details] [review] Bug 24807: Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowning out more relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Created attachment 103085 [details] [review] Bug 24807: Add database update script Created attachment 103086 [details] [review] Bug 24807: Update tests Created attachment 103087 [details] [review] Bug 24807: Add suppport for uncertain fields and ranges To test: 1 - Have some records with uncertain dates in the 008 19uu, 195u, etc. 2 - Index them in Elasticsearch 3 - Do a search that will return them 4 - Sort results by publication/copyright date 5 - Note odd results 6 - Apply patch 7 - Reindex 8 - Sorting should be improved Ok! Thanks for the references, will have a look at it. Rebased against latest master so that applies again. At first did not realize that one case is split up one subfield value into multiple ones. The way filter_callbacks works (it might be a bit overkill to allow multiple callbacks, but that is the way it is right now), is that the value returned is passed on to the next callback. So if you return an array the next callback (if one would exist) would expect a scalar and crash. I found a way around this by creating a new kind of callback, "tokenize_callbacks", so that the rest of the callback can keep their current function signature. Will be back with patch to demonstrate this in a while. On another note all sort fields are currently concatenated in the end, so "1963-2003" will end up as "1963 2003" in Elastic anyway. I'm not sure that I agree with this since sorting on numeric fields with multiple values actually can make sense (if sorting in descending order the highest value will be used by elastic and the other way around). We do this in the Perl code to be more specific, so could easily be "fixed". Created attachment 103163 [details] [review] Bug 24807: Refactor using tokenize_callbacks Created attachment 103277 [details] [review] Bug 24807: Simplify with new and imporved value_callbacks Realized that all of what tokenize_callbacks and filter_callbacks do could be expressed as value_callback if changing the processing of that callback to the one used by tokenize_callbacks (if effect allowing value callbacks to return a scalar, a list (replacing tokenize_callbacks) or an empty list (replacing filter_callbacks). So cleaned up the code with regards to this. *** Bug 24559 has been marked as a duplicate of this bug. *** Created attachment 104525 [details] [review] Bug 24807: Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowning out more relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 104526 [details] [review] Bug 24807: Add database update script Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 104527 [details] [review] Bug 24807: Update tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 104528 [details] [review] Bug 24807: Add suppport for uncertain fields and ranges To test: 1 - Have some records with uncertain dates in the 008 19uu, 195u, etc. 2 - Index them in Elasticsearch 3 - Do a search that will return them 4 - Sort results by publication/copyright date 5 - Note odd results 6 - Apply patch 7 - Reindex 8 - Sorting should be improved Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 104529 [details] [review] Bug 24807: Refactor using tokenize_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 104530 [details] [review] Bug 24807: Simplify with new and imporved value_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 104531 [details] [review] Bug 24807: (follow-up) Fix spelling Signed-off-by: Nick Clemens <nick@bywatersolutions.com> > return map { s/[u]/0/gr } ( $value =~ /[0-9u]{4}/g ); This line has some problems IMO: - It only considers years of 4 digits. I believe it is valid in MARC21 to write years with 3 digits or less, so it should be possible to index those values too. - In UNIMARC uncertain digits can be replaced by a blank (space) in some cases (100$a for instance : https://www.transition-bibliographique.fr/wp-content/uploads/2018/07/B100-6-2010.pdf [in french, sorry]) - Replacing the uncertain character by 0 can be problematic. 197u is not 1970. Ideally, searching for records published in 1975 should return records where publication date is "197u", right ? Elasticsearch has an integer_range data type that could be useful in this situation. What do you think ? (In reply to Julian Maurice from comment #29) > > return map { s/[u]/0/gr } ( $value =~ /[0-9u]{4}/g ); > > This line has some problems IMO: > - It only considers years of 4 digits. I believe it is valid in MARC21 to > write years with 3 digits or less, so it should be possible to index those > values too. > - In UNIMARC uncertain digits can be replaced by a blank (space) in some > cases (100$a for instance : > https://www.transition-bibliographique.fr/wp-content/uploads/2018/07/B100-6- > 2010.pdf [in french, sorry]) From what little I understand there, UNIMARC uses '#' as the unkown character? or '#' translates to blank? leading blanks should be preserved as 0? and trailing ranged as you say below? > - Replacing the uncertain character by 0 can be problematic. 197u is not > 1970. Ideally, searching for records published in 1975 should return records > where publication date is "197u", right ? Elasticsearch has an integer_range > data type that could be useful in this situation. What do you think ? That is interesting, how does the range affect sorting? Might it still be worthwhile to sort unknown/ranged dates either at start or end, so file the sort value as either 1970 or 1979? (In reply to Nick Clemens from comment #30) > From what little I understand there, UNIMARC uses '#' as the unkown > character? or '#' translates to blank? I think that '#' translates to blank. UNIMARC cataloguing plugin for 100$a in Koha uses spaces. > leading blanks should be preserved as 0? I think so. > and trailing ranged as you say below? I don't know... > > > - Replacing the uncertain character by 0 can be problematic. 197u is not > > 1970. Ideally, searching for records published in 1975 should return records > > where publication date is "197u", right ? Elasticsearch has an integer_range > > data type that could be useful in this situation. What do you think ? > > That is interesting, how does the range affect sorting? Might it still be > worthwhile to sort unknown/ranged dates either at start or end, so file the > sort value as either 1970 or 1979? I have not tested integer_range yet. Rethinking about this, maybe replacing "uncertain" by 0 is good enough for now. Support for uncertain dates as ranges can be added later. Created attachment 106768 [details] [review] Bug 24807: (follow-up) Add support for spaces as unknown characters Needs a small rebase. Created attachment 108382 [details] [review] Bug 24807: Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowning out more relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108383 [details] [review] Bug 24807: Add database update script Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108384 [details] [review] Bug 24807: Update tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108385 [details] [review] Bug 24807: Add suppport for uncertain fields and ranges To test: 1 - Have some records with uncertain dates in the 008 19uu, 195u, etc. 2 - Index them in Elasticsearch 3 - Do a search that will return them 4 - Sort results by publication/copyright date 5 - Note odd results 6 - Apply patch 7 - Reindex 8 - Sorting should be improved Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108386 [details] [review] Bug 24807: Refactor using tokenize_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108387 [details] [review] Bug 24807: Simplify with new and imporved value_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108388 [details] [review] Bug 24807: (follow-up) Fix spelling Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 108389 [details] [review] Bug 24807: (follow-up) Add support for spaces as unknown characters I am still learning about Elasticsearch. In order to test this patch set, I applied the patches and tried do reindex. When I run the reindex with -d, I have an error that doesn't appear without the patches: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ ./misc/search_tools/rebuild_elasticsearch.pl -d One or more ElasticSearch errors occurred when indexing documents at ./misc/search_tools/rebuild_elasticsearch.pl line 302. The sorting appears to work really well. (In reply to Katrin Fischer from comment #42) > I am still learning about Elasticsearch. In order to test this patch set, I > applied the patches and tried do reindex. > > When I run the reindex with -d, I have an error that doesn't appear without > the patches: > > kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ > ./misc/search_tools/rebuild_elasticsearch.pl -d > One or more ElasticSearch errors occurred when indexing documents at > ./misc/search_tools/rebuild_elasticsearch.pl line 302. > > > The sorting appears to work really well. Record #115 "Perl hacks" has a malformed 008 - this causes the error. The error does not occur when using ES6. I think this is okay as we expect errors if records have issues. We can use compare_es_to_db.pl to identify the records, or see bug 26310 for option to get more debug info Created attachment 109997 [details] [review] Bug 24807: Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowning out more relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109998 [details] [review] Bug 24807: Add database update script Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 109999 [details] [review] Bug 24807: Update tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110000 [details] [review] Bug 24807: Add suppport for uncertain fields and ranges To test: 1 - Have some records with uncertain dates in the 008 19uu, 195u, etc. 2 - Index them in Elasticsearch 3 - Do a search that will return them 4 - Sort results by publication/copyright date 5 - Note odd results 6 - Apply patch 7 - Reindex 8 - Sorting should be improved Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110001 [details] [review] Bug 24807: Refactor using tokenize_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110002 [details] [review] Bug 24807: Simplify with new and imporved value_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110003 [details] [review] Bug 24807: (follow-up) Fix spelling Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 110004 [details] [review] Bug 24807: (follow-up) Add support for spaces as unknown characters Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 20.11, thanks to everybody involved! There is a failing test, please fix ASAP t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t .. 1/3 # Looks like you planned 2 tests but ran 1. # Failed test 'update_index() tests' # at t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t line 98. Not a HASH reference at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Indexer.pm line 120. # Looks like your test exited with 255 just after 3. https://jenkins.koha-community.org/job/Koha_Master_U20/92/consoleFull Created attachment 110355 [details] [review] Bug 24807: (QA follow-up) Remove uneccessary tests These tests fail now, the code expects a real response from ES in Indexer.pm but these tests mock 'bulk' and so don't have the necessary fields. We are testing the same code above and can just add the _id == biblionumber test Follow-up pushed to master, thanks Nick! Created attachment 110884 [details] [review] Bug 24807: [20.05.x] Add "year" type to improve sorting behaviour Add a "year" search field type. Fields with this type will only retain values that looks like years, so invalid values such as whitespace or word characters will not be indexed. This for instance improves the behaviour when sorting by "date-of-publication". If all values are indexed, records with junk data instead of valid years will appear first among the search results, drowning out more relevant hits. If assigning this field the "year" type these records will instead always appear last, regarless of sort order. To test: 1) Have at least two biblios, one with a valid year in 008 (pos 7-10) and another with an invalid one ("uuuu" for example) 2) Perform a wildcard search (*) and sort results by publication date. 3) The record with invalid year of pulication in 008 should appear first 4) Apply patch and run database updates 5) Reindex ElasticSearch 6) Perform the same search as in 2) 7) The record with the invalid year should now appear last Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: Add database update script Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: Update tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: Add suppport for uncertain fields and ranges To test: 1 - Have some records with uncertain dates in the 008 19uu, 195u, etc. 2 - Index them in Elasticsearch 3 - Do a search that will return them 4 - Sort results by publication/copyright date 5 - Note odd results 6 - Apply patch 7 - Reindex 8 - Sorting should be improved Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: Refactor using tokenize_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: Simplify with new and imporved value_callbacks Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: (follow-up) Fix spelling Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: (follow-up) Add support for spaces as unknown characters Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Bug 24807: (QA follow-up) Remove uneccessary tests These tests fail now, the code expects a real response from ES in Indexer.pm but these tests mock 'bulk' and so don't have the necessary fields. We are testing the same code above and can just add the _id == biblionumber test backported to 20.05.x for 20.05.05 enhancement, not backported to 19.11.x |