Bug 29632 - Callnumber sorting is incorrect in Elasticsearch
Summary: Callnumber sorting is incorrect in Elasticsearch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 26051
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-03 13:48 UTC by Nick Clemens
Modified: 2023-12-28 20:42 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the sorting of call numbers when using Elasticsearch. Sorting will now work correctly for non-numeric call numbers, for example, E45 will now sort before E7.
Version(s) released in:
22.11.00, 22.05.05


Attachments
Bug 29632: Don't sort cn-sort numerically (4.62 KB, patch)
2022-06-28 13:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29632: Don't sort cn-sort numerically (4.67 KB, patch)
2022-06-29 21:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 29632: Don't sort cn-sort numerically (4.72 KB, patch)
2022-07-05 18:28 UTC, Michal Urban
Details | Diff | Splinter Review
Bug 29632: Unit tests (2.90 KB, patch)
2022-07-11 15:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29632: Don't sort cn-sort numerically (4.78 KB, patch)
2022-07-12 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29632: Unit tests (2.96 KB, patch)
2022-07-12 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29632: (QA follow-up) Add ENUM value to kohastructure.sql :-D (1.55 KB, patch)
2022-07-18 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29632: (QA follow-up) Fix COMMENT discrepancy on upgrade (1.21 KB, patch)
2022-08-04 12:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29632: [21.11.X] Don't sort cn-sort numerically (8.95 KB, patch)
2022-08-26 13:02 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 Nick Clemens 2021-12-03 13:48:05 UTC
It looks like we are not sorting cn_sort as a raw field, but sorting the analyzed version.

To recreate:
0 - Be using ES with Koha
1 - On records with single item, add callnumbers:
    VA65 E7 R63 1984
    VA65 E7 T35 1990
    VA65 E45 R67 1985
2 - Add public note 'shrimp' or something to make them easily searchable as a group
3 - Search for 'shrimp'
4 - Note E45 comes last, it should come first

Problem may be here:
1122 sub _sort_field {
1123     my ($self, $f) = @_;
1124 
1125     my $mappings = $self->get_elasticsearch_mappings();
1126     my $textField = defined $mappings->{data}{properties}{$f}{type} && $mappings->{data}{properties}{$f}{type} eq      'text';
1127     if (!defined $self->sort_fields()->{$f} || $self->sort_fields()->{$f}) {
1128         $f .= '__sort';
1129     } else {
1130         # We need to add '.raw' to text fields without a sort field,
1131         # otherwise it'll sort based on the tokenised form.
1132         $f .= '.raw' if $textField;
1133     }
1134     return $f;
1135 }

We need to enforce raw for cn-sort, or we need to index it unanalyzed
Comment 1 Erica Rohlfs 2021-12-03 15:13:31 UTC
Noting that I recreated this workflow on Koha version 20.11. My results returned step 4 as the second result. So, in my testing, Koha returned results in the following order: VA65 E7 T35 1990, VA65 E45 R67 1985, VA65 E7 R63 1984.
Comment 2 Heather 2021-12-08 17:21:18 UTC
We're having this problem on 21.05 and can be seen in our OPAC:
https://keys.bywatersolutions.com/cgi-bin/koha/opac-search.pl?q=callnum%2Cphr%3Ava65&offset=80&sort_by=call_number_asc

You'll see that VA65 E45 is still sorting *after* VA65 E7.

It's a big problem for us to continue to have the LC call numbers not sorted correctly--the staff can't sort correctly for paging lists, for example, and being a closed stack library, this impacts us daily, with every service request, where they're operating from Carts, searches, lists, and often generating their own paging/pull lists from various parts of Koha.
Comment 3 Nick Clemens 2022-06-28 13:50:36 UTC
Created attachment 136682 [details] [review]
Bug 29632: Don't sort cn-sort numerically

When defining our sort fields in we defined all as 'numeric'

For other string containing numbers this is likely correct, however,
for callnumbers it is not. e.g. E45 should sort before E7

This patch adds a new 'callnumber' type and deifnes this for cn-sort and
adds to the field maping a sort without numeric set

To test:
0 - Be using ES with Koha
1 - On records with single item, add callnumbers:
    VA65 E7 R63 1984
    VA65 E7 T35 1990
    VA65 E45 R67 1985
2 - Add public note 'shrimp' or something to make them easily searchable as a group
3 - Search for 'shrimp', sort by callnumber
4 - Note E45 comes last, it should come first
5 - Apply patch
6 - Reset ES mappings
7 - Reindex ES
8 - Repeat search
9 - Sorting should be correct when set to callnumber
Comment 4 David Nind 2022-06-29 21:56:06 UTC
Created attachment 136754 [details] [review]
Bug 29632: Don't sort cn-sort numerically

When defining our sort fields in we defined all as 'numeric'

For other string containing numbers this is likely correct, however,
for callnumbers it is not. e.g. E45 should sort before E7

This patch adds a new 'callnumber' type and deifnes this for cn-sort and
adds to the field maping a sort without numeric set

To test:
0 - Be using ES with Koha
1 - On records with single item, add callnumbers:
    VA65 E7 R63 1984
    VA65 E7 T35 1990
    VA65 E45 R67 1985
2 - Add public note 'shrimp' or something to make them easily searchable as a group
3 - Search for 'shrimp', sort by callnumber
4 - Note E45 comes last, it should come first
5 - Apply patch
6 - Reset ES mappings
7 - Reindex ES
8 - Repeat search
9 - Sorting should be correct when set to callnumber

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-06-29 22:00:53 UTC
Testing notes (using koha-testing-docker):

- After applying the patch (step 5) you need to run a database update (updatedatabase0
Comment 6 Michal Urban 2022-07-05 18:28:42 UTC
Created attachment 137182 [details] [review]
Bug 29632: Don't sort cn-sort numerically

When defining our sort fields in we defined all as 'numeric'

For other string containing numbers this is likely correct, however,
for callnumbers it is not. e.g. E45 should sort before E7

This patch adds a new 'callnumber' type and deifnes this for cn-sort and
adds to the field maping a sort without numeric set

To test:
0 - Be using ES with Koha
1 - On records with single item, add callnumbers:
    VA65 E7 R63 1984
    VA65 E7 T35 1990
    VA65 E45 R67 1985
2 - Add public note 'shrimp' or something to make them easily searchable as a group
3 - Search for 'shrimp', sort by callnumber
4 - Note E45 comes last, it should come first
5 - Apply patch
6 - Reset ES mappings
7 - Reindex ES
8 - Repeat search
9 - Sorting should be correct when set to callnumber

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Michal Urban <michalurban177@gmail.com>
Comment 7 Martin Renvoize 2022-07-06 11:04:36 UTC
Looks like we're missing a unit test here for the Koha/SearchEngine/Elasticsearch.pm change.
Comment 8 Nick Clemens 2022-07-11 15:57:35 UTC
Created attachment 137539 [details] [review]
Bug 29632: Unit tests

This patch adds unit tests, as well as changing existing test to use a mock
and read the data as passed in tests, rather than relying on what exists in
the DB
Comment 9 Martin Renvoize 2022-07-12 07:28:37 UTC
Created attachment 137605 [details] [review]
Bug 29632: Don't sort cn-sort numerically

When defining our sort fields in we defined all as 'numeric'

For other string containing numbers this is likely correct, however,
for callnumbers it is not. e.g. E45 should sort before E7

This patch adds a new 'callnumber' type and deifnes this for cn-sort and
adds to the field maping a sort without numeric set

To test:
0 - Be using ES with Koha
1 - On records with single item, add callnumbers:
    VA65 E7 R63 1984
    VA65 E7 T35 1990
    VA65 E45 R67 1985
2 - Add public note 'shrimp' or something to make them easily searchable as a group
3 - Search for 'shrimp', sort by callnumber
4 - Note E45 comes last, it should come first
5 - Apply patch
6 - Reset ES mappings
7 - Reindex ES
8 - Repeat search
9 - Sorting should be correct when set to callnumber

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Michal Urban <michalurban177@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2022-07-12 07:28:41 UTC
Created attachment 137606 [details] [review]
Bug 29632: Unit tests

This patch adds unit tests, as well as changing existing test to use a mock
and read the data as passed in tests, rather than relying on what exists in
the DB

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2022-07-12 07:29:00 UTC
Thanks Nick, Passing QA now :)
Comment 12 Tomás Cohen Arazi 2022-07-12 18:59:49 UTC
Shouldn't we add the index to the configuration on the update?
Comment 13 Tomás Cohen Arazi 2022-07-18 14:53:34 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Tomás Cohen Arazi 2022-07-18 15:10:03 UTC
Created attachment 137823 [details] [review]
Bug 29632: (QA follow-up) Add ENUM value to kohastructure.sql :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2022-08-04 12:57:49 UTC
Created attachment 138611 [details] [review]
Bug 29632: (QA follow-up) Fix COMMENT discrepancy on upgrade

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Lucas Gass 2022-08-23 17:36:36 UTC
Backported to 22.05.x for 22.05.05
Comment 17 Arthur Suzuki 2022-08-26 12:45:31 UTC
conflicts in templates when trying to apply on 21.11.
Provide a backport if needed.
Comment 18 Nick Clemens 2022-08-26 13:02:36 UTC
Created attachment 139827 [details] [review]
Bug 29632: [21.11.X] Don't sort cn-sort numerically

When defining our sort fields in we defined all as 'numeric'

For other string containing numbers this is likely correct, however,
for callnumbers it is not. e.g. E45 should sort before E7

This patch adds a new 'callnumber' type and deifnes this for cn-sort and
adds to the field maping a sort without numeric set

To test:
0 - Be using ES with Koha
1 - On records with single item, add callnumbers:
    VA65 E7 R63 1984
    VA65 E7 T35 1990
    VA65 E45 R67 1985
2 - Add public note 'shrimp' or something to make them easily searchable as a group
3 - Search for 'shrimp', sort by callnumber
4 - Note E45 comes last, it should come first
5 - Apply patch
6 - Reset ES mappings
7 - Reindex ES
8 - Repeat search
9 - Sorting should be correct when set to callnumber

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Michal Urban <michalurban177@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29632: Unit tests

This patch adds unit tests, as well as changing existing test to use a mock
and read the data as passed in tests, rather than relying on what exists in
the DB

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 29632: (QA follow-up) Add ENUM value to kohastructure.sql :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 29632: (QA follow-up) Fix COMMENT discrepancy on upgrade

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Nick Clemens 2022-08-26 13:03:28 UTC
(In reply to Arthur Suzuki from comment #17)
> conflicts in templates when trying to apply on 21.11.
> Provide a backport if needed.

Patch supplied