Bug 24807 - Add "year" type to improve sorting by publication date
Summary: Add "year" type to improve sorting by publication date
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact:
URL:
Keywords:
: 24559 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-03-05 09:40 UTC by David Gustafsson
Modified: 2021-06-14 21:31 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.05


Attachments
Bug 24807: Add "year" type to improve sorting behaviour (8.88 KB, patch)
2020-03-05 10:05 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Add "year" type to improve sorting behaviour (8.89 KB, patch)
2020-03-05 10:06 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Add database update script (1.20 KB, patch)
2020-03-05 10:36 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Update tests (3.89 KB, patch)
2020-04-08 09:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add suppport for uncertain fields and ranges (2.07 KB, patch)
2020-04-08 09:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add "year" type to improve sorting behaviour (9.03 KB, patch)
2020-04-16 12:53 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Add database update script (1.20 KB, patch)
2020-04-16 12:53 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Update tests (4.18 KB, patch)
2020-04-16 12:53 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Add suppport for uncertain fields and ranges (2.07 KB, patch)
2020-04-16 12:53 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Refactor using tokenize_callbacks (6.50 KB, patch)
2020-04-17 13:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Simplify with new and imporved value_callbacks (4.63 KB, patch)
2020-04-20 11:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 24807: Add "year" type to improve sorting behaviour (9.10 KB, patch)
2020-05-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add database update script (1.26 KB, patch)
2020-05-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Update tests (4.24 KB, patch)
2020-05-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add suppport for uncertain fields and ranges (2.12 KB, patch)
2020-05-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Refactor using tokenize_callbacks (6.56 KB, patch)
2020-05-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Simplify with new and imporved value_callbacks (4.69 KB, patch)
2020-05-07 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: (follow-up) Fix spelling (1.68 KB, patch)
2020-05-07 16:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: (follow-up) Add support for spaces as unknown characters (3.44 KB, patch)
2020-07-10 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add "year" type to improve sorting behaviour (9.12 KB, patch)
2020-08-17 10:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add database update script (1.26 KB, patch)
2020-08-17 10:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Update tests (4.24 KB, patch)
2020-08-17 10:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add suppport for uncertain fields and ranges (2.12 KB, patch)
2020-08-17 10:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Refactor using tokenize_callbacks (6.56 KB, patch)
2020-08-17 10:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Simplify with new and imporved value_callbacks (4.69 KB, patch)
2020-08-17 10:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: (follow-up) Fix spelling (1.68 KB, patch)
2020-08-17 10:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: (follow-up) Add support for spaces as unknown characters (3.44 KB, patch)
2020-08-17 10:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: Add "year" type to improve sorting behaviour (9.23 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: Add database update script (1.32 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: Update tests (4.35 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: Add suppport for uncertain fields and ranges (2.18 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: Refactor using tokenize_callbacks (6.68 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: Simplify with new and imporved value_callbacks (4.75 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: (follow-up) Fix spelling (1.74 KB, patch)
2020-09-12 22:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: (follow-up) Add support for spaces as unknown characters (3.58 KB, patch)
2020-09-12 22:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24807: (QA follow-up) Remove uneccessary tests (2.66 KB, patch)
2020-09-18 10:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24807: [20.05.x] Add "year" type to improve sorting behaviour (18.26 KB, patch)
2020-09-28 16:51 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2020-03-05 09:40:55 UTC
When sorting on date-of-publication (a year, field 008, positions 7-10), the current behavior when the field contains an invalid year (containing word characters like "uuuu" or just whitespace), these results appear first when sorting in descending order. Invalid values like these should really not be indexed at all. If so (having null values instead) they will always appear last when sorting, regardless of sort order. This fix adds a "year" search field type, that will reject any non-year values, resulting in a more user friendly sorting behavior.
Comment 1 David Gustafsson 2020-03-05 10:05:39 UTC Comment hidden (obsolete)
Comment 2 David Gustafsson 2020-03-05 10:06:44 UTC
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
Comment 3 David Gustafsson 2020-03-05 10:36:19 UTC
Created attachment 100176 [details] [review]
Bug 24807: Add database update script
Comment 4 Nick Clemens 2020-04-08 09:58:52 UTC
Created attachment 102547 [details] [review]
Bug 24807: Update tests
Comment 5 Nick Clemens 2020-04-08 09:58:55 UTC
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
Comment 6 Nick Clemens 2020-04-08 10:01:53 UTC
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
Comment 7 David Gustafsson 2020-04-15 10:12:29 UTC
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.
Comment 8 David Gustafsson 2020-04-15 16:20:25 UTC
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.
Comment 9 Nick Clemens 2020-04-16 10:49:10 UTC
(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
Comment 10 David Gustafsson 2020-04-16 12:53:05 UTC
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
Comment 11 David Gustafsson 2020-04-16 12:53:09 UTC
Created attachment 103085 [details] [review]
Bug 24807: Add database update script
Comment 12 David Gustafsson 2020-04-16 12:53:13 UTC
Created attachment 103086 [details] [review]
Bug 24807: Update tests
Comment 13 David Gustafsson 2020-04-16 12:53:17 UTC
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
Comment 14 David Gustafsson 2020-04-16 12:55:02 UTC
Ok! Thanks for the references, will have a look at it. Rebased against latest master so that applies again.
Comment 15 David Gustafsson 2020-04-16 13:59:24 UTC
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.
Comment 16 David Gustafsson 2020-04-16 14:03:13 UTC
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).
Comment 17 David Gustafsson 2020-04-16 14:03:51 UTC
We do this in the Perl code to be more specific, so could easily be "fixed".
Comment 18 David Gustafsson 2020-04-17 13:32:22 UTC
Created attachment 103163 [details] [review]
Bug 24807: Refactor using tokenize_callbacks
Comment 19 David Gustafsson 2020-04-20 11:11:41 UTC
Created attachment 103277 [details] [review]
Bug 24807: Simplify with new and imporved value_callbacks
Comment 20 David Gustafsson 2020-04-20 11:16:29 UTC
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.
Comment 21 Nick Clemens 2020-05-06 13:16:07 UTC
*** Bug 24559 has been marked as a duplicate of this bug. ***
Comment 22 Nick Clemens 2020-05-07 16:27:43 UTC
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>
Comment 23 Nick Clemens 2020-05-07 16:27:46 UTC
Created attachment 104526 [details] [review]
Bug 24807: Add database update script

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2020-05-07 16:27:49 UTC
Created attachment 104527 [details] [review]
Bug 24807: Update tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2020-05-07 16:27:52 UTC
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>
Comment 26 Nick Clemens 2020-05-07 16:27:56 UTC
Created attachment 104529 [details] [review]
Bug 24807: Refactor using tokenize_callbacks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2020-05-07 16:27:59 UTC
Created attachment 104530 [details] [review]
Bug 24807: Simplify with new and imporved value_callbacks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2020-05-07 16:28:02 UTC
Created attachment 104531 [details] [review]
Bug 24807: (follow-up) Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Julian Maurice 2020-06-05 11:44:48 UTC
> 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 ?
Comment 30 Nick Clemens 2020-06-05 16:15:05 UTC
(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?
Comment 31 Julian Maurice 2020-06-12 09:30:53 UTC
(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.
Comment 32 Nick Clemens 2020-07-10 13:16:41 UTC
Created attachment 106768 [details] [review]
Bug 24807: (follow-up) Add support for spaces as unknown characters
Comment 33 Katrin Fischer 2020-08-16 21:59:20 UTC
Needs a small rebase.
Comment 34 Nick Clemens 2020-08-17 10:02:42 UTC
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>
Comment 35 Nick Clemens 2020-08-17 10:02:47 UTC
Created attachment 108383 [details] [review]
Bug 24807: Add database update script

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Nick Clemens 2020-08-17 10:02:54 UTC
Created attachment 108384 [details] [review]
Bug 24807: Update tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 Nick Clemens 2020-08-17 10:02:59 UTC
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>
Comment 38 Nick Clemens 2020-08-17 10:03:03 UTC
Created attachment 108386 [details] [review]
Bug 24807: Refactor using tokenize_callbacks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Nick Clemens 2020-08-17 10:03:10 UTC
Created attachment 108387 [details] [review]
Bug 24807: Simplify with new and imporved value_callbacks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Nick Clemens 2020-08-17 10:03:15 UTC
Created attachment 108388 [details] [review]
Bug 24807: (follow-up) Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Nick Clemens 2020-08-17 10:03:21 UTC
Created attachment 108389 [details] [review]
Bug 24807: (follow-up) Add support for spaces as unknown characters
Comment 42 Katrin Fischer 2020-08-22 22:39:10 UTC
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.
Comment 43 Nick Clemens 2020-09-02 12:41:55 UTC
(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
Comment 44 Katrin Fischer 2020-09-12 22:30:04 UTC
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>
Comment 45 Katrin Fischer 2020-09-12 22:30:13 UTC
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>
Comment 46 Katrin Fischer 2020-09-12 22:30:20 UTC
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>
Comment 47 Katrin Fischer 2020-09-12 22:30:28 UTC
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>
Comment 48 Katrin Fischer 2020-09-12 22:30:38 UTC
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>
Comment 49 Katrin Fischer 2020-09-12 22:30:45 UTC
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>
Comment 50 Katrin Fischer 2020-09-12 22:30:53 UTC
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>
Comment 51 Katrin Fischer 2020-09-12 22:31:01 UTC
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>
Comment 52 Jonathan Druart 2020-09-18 09:53:28 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 53 Jonathan Druart 2020-09-18 10:29:25 UTC
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
Comment 54 Nick Clemens 2020-09-18 10:37:43 UTC
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
Comment 55 Jonathan Druart 2020-09-23 20:16:21 UTC
Follow-up pushed to master, thanks Nick!
Comment 56 Nick Clemens 2020-09-28 16:51:28 UTC
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
Comment 57 Lucas Gass 2020-10-16 14:37:57 UTC
backported to 20.05.x for 20.05.05
Comment 58 Aleisha Amohia 2020-10-20 01:25:07 UTC
enhancement, not backported to 19.11.x