Bug 27299 - Zebra phrase register is incorrectly tokenized when using ICU
Summary: Zebra phrase register is incorrectly tokenized when using ICU
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Zebra (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact:
URL:
Keywords:
: 13751 (view as bug list)
Depends on: 10729
Blocks: 11158 18017
  Show dependency treegraph
 
Reported: 2020-12-22 22:56 UTC by David Cook
Modified: 2022-06-06 20:25 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Previously, Zebra indexing in ICU mode was incorrectly tokenizing text for the "p" register. This meant that particular phrase searches were not working as expected. With this change, phrase searching works the same in ICU and CHR modes.
Version(s) released in:
21.05.00,20.11.02


Attachments
Bug 27299: Remove tokenization for phrase register/index with ICU (995 bytes, patch)
2020-12-22 22:58 UTC, David Cook
Details | Diff | Splinter Review
Bib record used for testing auth linking (3.73 KB, application/xml)
2021-01-04 23:30 UTC, David Cook
Details
Bug 27299: Remove tokenization for phrase register/index with ICU (1.04 KB, patch)
2021-01-12 10:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27299: Remove tokenization for phrase register/index with ICU (1.10 KB, patch)
2021-01-15 09:49 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-12-22 22:56:39 UTC
Recently, I noticed issues with "exact" matching for authority linking when using Zebra ICU. 

I've documented those issues upstream on the idzebra project on Github:
https://github.com/indexdata/idzebra/issues/24

Adam Dickmeiss and I are still working through this issue, but it seems very likely to me that the issue is that we are tokenizing strings for the "p" register when we should not be. 

Looking at Zebra CHR, the "p" register is not tokenized. According to Zebra's own documentation (https://software.indexdata.com/zebra/doc/querymodel-zebra.html#querymodel-pqf-apt-mapping-structuretype), the "p" register is supposed to be "Character normalized, but not tokenized index for phrase matches". 

I'm still waiting for Adam to confirm my solution, but I've opened this bug report to track things on the Koha side, and to include a patch which I hope will resolve these problems.
Comment 1 David Cook 2020-12-22 22:58:57 UTC
Created attachment 114646 [details] [review]
Bug 27299: Remove tokenization for phrase register/index with ICU

This patch removes tokenization for phrase register/index when
using ICU.

This should cause "exact" searches to work the same way with ICU
as they do with CHR.
Comment 2 David Cook 2020-12-22 23:12:40 UTC
Here's a line for testing phrases-icu.xml:

echo "Gontarski, S. E." | yaz-icu -c ./etc/zebradb/etc/phrases-icu.xml

With the patch, I get the following:

1 1 'gontarski s e
' 'Gontarski S E
'

Without the patch, I get the following:
1 1 'gontarski ' 'Gontarski '
2 1 's ' 'S '
3 1 'e
' 'E
'
Comment 3 David Cook 2020-12-22 23:14:53 UTC
Actually, I have tested my change using the test_files.zip that I provided at https://github.com/indexdata/idzebra/issues/24, and this patch works well.

I'm still going to wait to hear from Indexdata, but I think this is probably the right approach.

Once I've confirmed with them, then I could look at adding a more Koha-centric test plan.
Comment 4 David Cook 2021-01-04 00:28:51 UTC
Based on https://github.com/indexdata/idzebra/issues/24#issuecomment-751260285, I think that Adam is confirming that we should not be using tokenization in phrases-icu.xml, but that they're not going to change the default phrases-icu.xml file in Zebra, because they're worried that it would negatively impact users upgrading.

So I think at this point we can move forward with my patch.
Comment 5 Fridolin Somers 2021-01-04 08:58:12 UTC
I think you need to restart zebra server and indexer after applying patch
Comment 6 David Cook 2021-01-04 22:39:02 UTC
(In reply to Fridolin Somers from comment #5)
> I think you need to restart zebra server and indexer after applying patch

Oops. I didn't include a test plan for Koha! I'll remedy that.
Comment 7 David Cook 2021-01-04 23:05:49 UTC
Test plan:

0. Do not apply the patch yet

1. vi /etc/koha/zebradb/etc/default.idx
2. Replace the first charmap line with "icuchain words-icu.xml"
3. Replace the second charmap line with "icuchain phrases-icu.xml"
4. koha-zebra --stop kohadev
5. pkill zebrasrv
6. koha-zebra --start kohadev
7. koha-indexer --restart kohadev
8. koha-rebuild-zebra -f kohadev

9. Go to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=au%2Cphr%2Cext%3DE
10. Note that 6 results are returned

11. Apply the patch
12. cp /etc/koha/zebradb/etc/phrases-icu.xml /etc/koha/zebradb/etc/phrases-icu.bak
13. cp etc/zebradb/etc/phrases-icu.xml /etc/koha/zebradb/etc/phrases-icu.xml
14. koha-zebra --stop kohadev
15. pkill zebrasrv
16. koha-zebra --start kohadev
17. koha-indexer --restart kohadev
18. koha-rebuild-zebra -f kohadev

19. Go to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=au%2Cphr%2Cext%3DE
20. Note that 0 results are returned

21. Go to http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=au%2Cphr%2Cext%3DGontarski%2C+S+E
22. Note that you're redirected to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=25&found1=1
Comment 8 David Cook 2021-01-04 23:06:54 UTC
Admittedly this test plan targets biblio searching rather than authority searching, but that's because the user has some control over biblio searching and the authority searching is terrible (and misleading)...

But I should do something to prove the authority linking...
Comment 9 David Cook 2021-01-04 23:30:27 UTC
Alternate test plan:

0a. Do not apply the patch yet
0b. Change "BiblioAddsAuthorities" system preference to "allow"

1. vi /etc/koha/zebradb/etc/default.idx
2. Replace the first charmap line with "icuchain words-icu.xml"
3. Replace the second charmap line with "icuchain phrases-icu.xml"
4. koha-zebra --stop kohadev
5. pkill zebrasrv
6. koha-zebra --start kohadev
7. koha-indexer --restart kohadev
8. koha-rebuild-zebra -f kohadev

9. Go to http://localhost:8081/cgi-bin/koha/tools/stage-marc-import.pl
10. Choose bib-24.marcxml
11. Click "Upload file"

12. Choose "KohaBiblio (999$c)" for "Record matching rule"
13. Choose "No" for "Check for embedded item record data"
14. Click "Stage for import"
15. Click "Manage staged records"
16. Click "Import this batch into the catalog"

17. In a new tab, go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=25 
18. Hover over "Gontarski, Bad" and note that it shows the following URL: http://localhost:8081/cgi-bin/koha/authorities/detail.pl?authid=236 
19. Go to http://localhost:8081/cgi-bin/koha/authorities/detail.pl?authid=236 
20. Note this authority is for "Gontarski, S. E."
21. Click on "Used in 1 record(s)"
22. Note that you're taken to the biblio record with a Contributor of "Gontarski, Bad"
23. Click "Undo import into catalog" on the import page

24. Apply the patch
25. cp /etc/koha/zebradb/etc/phrases-icu.xml /etc/koha/zebradb/etc/phrases-icu.bak
26. cp etc/zebradb/etc/phrases-icu.xml /etc/koha/zebradb/etc/phrases-icu.xml
27. koha-zebra --stop kohadev
28. pkill zebrasrv
29. koha-zebra --start kohadev
30. koha-indexer --restart kohadev
31. koha-rebuild-zebra -f kohadev

32. On the import page, click "Import this batch into the catalog"
33. Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=25
34. Hover over "Gontarski, Bad" and note it shows the following URL: http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=au:%22Gontarski%2C%20Bad.%22
35. Go to http://localhost:8081/cgi-bin/koha/authorities/detail.pl?authid=236
36. Note that it says "This authority is not used in any records"
Comment 10 David Cook 2021-01-04 23:30:51 UTC
Created attachment 114827 [details]
Bib record used for testing auth linking

Bib record used for testing auth linking
Comment 11 David Cook 2021-01-04 23:34:18 UTC
Note that the "p" register very rarely gets used in Koha. It's only when a "complete field" search is done, which basically is only an "exact" search.

"Phrase" searching in Koha actually uses the "w" register.

For more info, see https://software.indexdata.com/zebra/doc/querymodel-zebra.html#querymodel-pqf-apt-mapping-structuretype
Comment 12 David Cook 2021-01-05 22:28:02 UTC
*** Bug 13751 has been marked as a duplicate of this bug. ***
Comment 13 Martin Renvoize 2021-01-12 10:09:36 UTC
Created attachment 115061 [details] [review]
Bug 27299: Remove tokenization for phrase register/index with ICU

This patch removes tokenization for phrase register/index when
using ICU.

This should cause "exact" searches to work the same way with ICU
as they do with CHR.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-01-12 10:10:22 UTC
This makes allot of sense and having now tested, I agree the tokenization was the issue.

Signing off.. I believe Frido is working through testing too so hopefully he can provide a quick QA :)
Comment 15 David Cook 2021-01-12 22:45:16 UTC
(In reply to Martin Renvoize from comment #14)
> This makes allot of sense and having now tested, I agree the tokenization
> was the issue.
> 
> Signing off.. I believe Frido is working through testing too so hopefully he
> can provide a quick QA :)

Excellent!
Comment 16 Fridolin Somers 2021-01-15 09:49:36 UTC
Created attachment 115185 [details] [review]
Bug 27299: Remove tokenization for phrase register/index with ICU

This patch removes tokenization for phrase register/index when
using ICU.

This should cause "exact" searches to work the same way with ICU
as they do with CHR.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 17 David Cook 2021-01-18 01:31:43 UTC
Should this be Passed QA now or still Signed Off?
Comment 18 Martin Renvoize 2021-01-19 12:17:15 UTC
I think with Fridolin and I haveing both tested we can safely PQA this.
Comment 19 Jonathan Druart 2021-01-19 15:24:19 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 20 Fridolin Somers 2021-01-20 07:29:32 UTC
Pushed to 20.11.x for 20.11.02
Comment 21 Fridolin Somers 2021-01-20 07:30:15 UTC
We need a release note to explain this
Comment 22 David Cook 2021-01-21 04:58:47 UTC
(In reply to Fridolin Somers from comment #21)
> We need a release note to explain this

Done :D
Comment 23 Andrew Fuerste-Henry 2021-01-29 19:30:48 UTC
Pushed to 20.05.x for 20.05.09
Comment 24 Victor Grousset/tuxayo 2021-02-01 22:42:23 UTC
Backported: Pushed to 19.11.x branch for 19.11.15
Comment 25 David Cook 2021-02-01 22:55:23 UTC
You know... in hindsight... this might not be a good one to backport. 

If we push out this change and people install it via a package upgrade without paying attention, their Zebra search is going to break until they do a full Zebra re-index.

That could make for a lot of very unhappy confused people.
Comment 26 David Cook 2021-02-01 22:56:07 UTC
The Zebrasrv will also need a restart. I'm not sure if we actually do that when upgrading Koha...
Comment 27 Fridolin Somers 2021-02-03 15:57:22 UTC
20.11.x is still young so I think this fix can be into it.
But maybe not in older branches.
Comment 28 Jonathan Druart 2021-02-03 16:10:56 UTC
(In reply to Fridolin Somers from comment #27)
> 20.11.x is still young so I think this fix can be into it.
> But maybe not in older branches.

How would they know a reindex is needed? You should at least add a big warning during the update DB process.
Comment 29 Victor Grousset/tuxayo 2021-02-03 19:17:52 UTC
> 20.11.x is still young

Hea lists 750 Kohas 20.11.x
Comment 30 Victor Grousset/tuxayo 2021-02-03 19:19:32 UTC
Reverted from 19.11.x
Comment 31 Andrew Fuerste-Henry 2021-02-03 19:52:24 UTC
Reverted from 20.05