Bug 10729 - Support phrases-icu.xml in install and upgrade
Summary: Support phrases-icu.xml in install and upgrade
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 27299
  Show dependency treegraph
 
Reported: 2013-08-14 15:19 UTC by Colin Campbell
Modified: 2021-01-04 00:03 UTC (History)
12 users (show)

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


Attachments
Proposed patch (2.67 KB, patch)
2013-08-14 15:35 UTC, Colin Campbell
Details | Diff | Splinter Review
bug 10729 add phrases configuration for icu (2.72 KB, patch)
2013-09-12 14:46 UTC, Paola Rossi
Details | Diff | Splinter Review
Amended Patch (2.71 KB, patch)
2013-10-03 15:08 UTC, Colin Campbell
Details | Diff | Splinter Review
[PASSED QA] Bug 10729: Add phrases configuration for icu (3.13 KB, patch)
2013-10-27 18:21 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2013-08-14 15:19:22 UTC
While resolving some indexing probs for customers using icu in zebra, noted that recommendations for using icu suggested copying phrases-icu.xml from the zerbra install into koha. That procedure is a bit at odds with standard install and upgrade procedure. Need to add a useable phrases-icu.xml to koha and to ensure that install now updates default.idx correctly
Comment 1 Colin Campbell 2013-08-14 15:35:28 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2013-09-12 14:46:47 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-09-22 20:40:56 UTC
Hi Colin,

can you explain what difference this will make in using Koha with ICU indexing? What is the problem and how can I reproduce it?
Comment 4 Zeno Tajoli 2013-09-24 09:42:47 UTC
Hi Katrin,

the ICU suppot is mandatory when you mix two or more alphabet in you bibliographic data.
The patch of Colin is about phrases indexing.

To index ICU needs trasformation, and it is better to have a trasformation for word and a different trasformation for phrases.

The path insert a more correct trasformation for phrases.
Comment 5 Katrin Fischer 2013-09-24 09:45:15 UTC
Hi Zeno, thx for your explanation. Can someone please add a step by step test plan for this?
Comment 6 Zeno Tajoli 2013-09-24 11:21:18 UTC
To test the patch we have done:

First time
a)Installed it before to do 'perl Makefile.PL'
b)Install with ICU
c)Catalogue a book
d)Search it with 'title phrase'

Second time
a)Install a master without the patth and Install with ICU
b)Install the patch and do a make upgrade, see if the new config is installed

This test doesn't show you the difference between previous setup and the patch setup.
I think that to see the difference we need an help form Colin
Comment 7 Colin Campbell 2013-09-24 13:37:47 UTC
One site found they were getting very inconsistent results between searches. In trying to locate what might be the problem they found that most accounts of how to enable icu in koha on the web suggest copying the files ( words and phrases ) from zebra and editing the configs. i.e. shortcircuiting the koha install/upgrade process (and having to redo your changes after a standard upgrade).

Site in question felt that having copied phrases their problems went away, but as the problem was erratic it hard to say conclusively what fixed it. (There is an argument for not stripping whitespace in phrase searches). So I'm not saying that this fixes a specific indexing problem. Its intention was to allow icu to be enabled without having to break the standard install/upgrade procedure.

The test is whether with icu enabled the two files words-icu.xml and phrases-icu.xml get installed and default.idx references them on install and preserved on make upgrade
Comment 8 Katrin Fischer 2013-09-28 11:05:13 UTC
It looks to me like this doesn't touch files relevant for the packages, but wonder if it should - I am adding Robin in CC to take a look.
Comment 9 Robin Sheat 2013-09-30 03:55:53 UTC
Should be fine, they get installed by the makefile, and building packages runs that, so they get picked up.
Comment 10 Katrin Fischer 2013-09-30 18:41:17 UTC
Thx for taking a look Robin.
Comment 11 Katrin Fischer 2013-09-30 20:11:27 UTC
I am sorry, but this doesn't work for me. 

My installation uses DOM indexing (authority and bibliographic records) and ICU.

What I did:

- Reindexed my date, search works correctly
- Applied patch
- perl Makefile.PL --prev-install-log ../koha-dev/misc/koha-install-log 
- make upgrade
- restarted Zebra server

Then tried to reindex:
[~/kohaclone (98-10729-icuphrase)⚡]> ./misc/migration_tools/rebuild_zebra.pl -b -v -r 
Zebra configuration information
================================
Zebra biblio directory      = /home/katrin/koha-dev/var/lib/zebradb/biblios
Zebra authorities directory = /home/katrin/koha-dev/var/lib/zebradb/authorities
Koha directory              = /home/katrin/kohaclone
BIBLIONUMBER in :     999$c
BIBLIOITEMNUMBER in : 999$d
================================
skipping authorities
====================
exporting biblio
====================
101..................................................................................................
Records exported: 118
====================
REINDEXING zebra
====================
*** stack smashing detected ***: zebraidx terminated
======= Backtrace: =========
... lots of unhappiness ...

Without the patch the same database reidexes without any problems.
Comment 12 Colin Campbell 2013-10-02 09:43:16 UTC
(In reply to Katrin Fischer from comment #11)
> I am sorry, but this doesn't work for me. 
> 
What versions of yaz and zebra are you using? also what is the output of icu-config --version ?
Comment 13 Katrin Fischer 2013-10-02 09:46:16 UTC
Hi Colin, I can't check before tonight, but maybe this might give you an idea - I have installed all dependencies using the koha-common packages under Ubuntu.
Comment 14 Katrin Fischer 2013-10-03 10:03:21 UTC
icu-config wasn't installed.
sudo apt-get install libicu-dev
icu-config --version
4.8.1.1
Zebra 2.0.44 
I was not sure how to get he yaz version, so this is what I did:
yaz-client
zversion
zversion is 3
Comment 15 Colin Campbell 2013-10-03 11:47:41 UTC
(In reply to Katrin Fischer from comment #14)
> icu-config wasn't installed.
> sudo apt-get install libicu-dev
> icu-config --version
> 4.8.1.1
> Zebra 2.0.44 
> I was not sure how to get he yaz version, so this is what I did:
> yaz-client
> zversion
> zversion is 3

I suspect its the bounds checking bug in zebra fixed in 2.0.51. I'll try building an older version to confirm

Here's the zebra changelog:
--- 2.0.51 2012/05/31

Fix buffer overrun for terms using ICU normalization.

Increase max size of indexing term from 256 to 512.
Comment 16 Colin Campbell 2013-10-03 15:08:08 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-10-03 15:09:57 UTC
Thx Colin!
Comment 18 Martin Renvoize 2013-10-17 18:28:15 UTC
OK, so lets add some background to this bug.

We have many customers using both ICU and DOM with zebra, and we found that phrase searching was broken.  A phrase search would end up only actually searching upon the first word in the phrase and as such return rather unspecific results.

This was reasonably apparent in the clients as 'bad results' in search.. but that's very objective.

It's also apparent when using the link bibs to authorities script and this is much more provable.

The link bibs to authorities script uses the exact search type that this patch affects, and we found that applying the pathc would drastically improve matching in the default mode of the script.

Test plan to follow... once I've got Ian to give me some exact examples.
Comment 19 Katrin Fischer 2013-10-17 19:58:31 UTC
Thx Martin!
Comment 20 Martin Renvoize 2013-10-22 00:26:42 UTC
This patch should affect linking of authorities where multiple similar authorities exist, i.e.

Law Agency
Law Agency Great Britian
Law Agency Germany
Law Agency France

Before the patch, these authorities would all be matched during a search for 'Law Agency' and thus would a link would not be created.  With the patch, the full phrase is searched correctly and so an exact match should happen, thus a link would be created.

The overall upshot, is that if you have a catalogue liek above, then running link_bibs script sohuld return many more posotive actions after the patch is applied than compared to before the patch is applied
Comment 21 Katrin Fischer 2013-10-27 18:21:39 UTC
Created attachment 22451 [details] [review]
[PASSED QA] Bug 10729: Add phrases configuration for icu

Add a separate phrases-icu.xml for phrase indexes
The file is based on that distributed with zebra
with a couple of additions to reflect Koha usage

This patch adds a separate tokenizer variable
for phrase indexes so that default.idx is
correctly rewritten for sites using icu
indexing

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
- Applied patch
- perl Makefile.PL --prev-install-log ../koha-dev/misc/koha-install-log
- make upgrade
- Restarted Zebra server
- Did a full reindex of bibliographic and authorities
- Checked various searches
- Links records to authorities
- Checked created links work correctly

I couldn't find a regression with this patch.
Passes all tests and QA script.
Comment 22 Mathieu Saby 2013-11-04 15:12:18 UTC
It seems very interesting. Will it be in 3.14 ?

Mathieu
Comment 23 Galen Charlton 2014-05-05 04:15:14 UTC
Pushed to master.  Thanks, Colin!
Comment 24 Pongtawat 2014-05-29 09:53:30 UTC
I still got stack smashing with the updated phrases-icu.xml.

My system:
 Koha 3.12.12 on Ubuntu 12.04 amd64
 idzebra-2.0 (2.0.44-3)
 libyaz4 (4.2.18-1build1)

However, if I specify words-icu.xml for phrase index instead of phrases-icu.xml, the indexing will finish fine. 

Error:

Records exported: 476647
====================
REINDEXING zebra
====================
*** stack smashing detected ***: zebraidx terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x37)[0x7f6b3ce9df47]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x0)[0x7f6b3ce9df10]
/usr/lib/libidzebra-2.0.so.0(+0x17007)[0x7f6b3d4f8007]
/usr/lib/libidzebra-2.0.so.0(+0x1712b)[0x7f6b3d4f812b]
/usr/lib/libidzebra-2.0.so.0(heap_cread_item2+0xf2)[0x7f6b3d4f82b2]
/usr/lib/libidzebra-2.0.so.0(insert_leaf+0x21a)[0x7f6b3d52395a]
/usr/lib/libidzebra-2.0.so.0(insert_int+0x756)[0x7f6b3d5249f6]
/usr/lib/libidzebra-2.0.so.0(isamb_merge+0xc3)[0x7f6b3d524cf3]
/usr/lib/libidzebra-2.0.so.0(heap_inpb+0x193)[0x7f6b3d4f8a23]
/usr/lib/libidzebra-2.0.so.0(zebra_index_merge+0x258)[0x7f6b3d4f8e38]
/usr/lib/libidzebra-2.0.so.0(+0x298df)[0x7f6b3d50a8df]
/usr/lib/libidzebra-2.0.so.0(zebra_end_transaction+0x11d)[0x7f6b3d50e26d]
/usr/lib/libidzebra-2.0.so.0(zebra_end_trans+0x31)[0x7f6b3d50e3d1]
zebraidx[0x401884]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x7f6b3cdb476d]
zebraidx[0x401a0d]
Comment 25 Zeno Tajoli 2014-05-29 11:17:13 UTC
Reading this thread:
http://koha.1045719.n5.nabble.com/Stack-Smashing-td3069232.html

it seems something connect with Ubuntu.
Could you try to test in Debian ?
Comment 26 Pongtawat 2014-06-02 10:23:08 UTC
I did some test by starting with phrases-icu.xml and change it towards words-icu.xml. Here is what I got:

1. Adding the first two transliterate rules in words-icu.xml to phrases-icu.xml.
--> Stack Smashed.

2. Changing this line:

<transform rule="[:Punctuation:] Remove"/>

to

<transform rule="[[:WhiteSpace:][:Punctuation:]] Remove"/>

Now it index fine.

So, if I just add the WhiteSpace remove rule to phrases-icu.xml, what effect it will cause?
Comment 27 Fridolin Somers 2014-06-03 09:20:33 UTC
(In reply to Pongtawat from comment #26)
> So, if I just add the WhiteSpace remove rule to phrases-icu.xml, what effect it will cause?

I think you must open a new bug.
Comment 28 Fridolin Somers 2014-06-03 09:43:17 UTC
Pushed to 3.14.x, will be in 3.14.08
Comment 29 Zeno Tajoli 2014-06-04 10:31:44 UTC
(In reply to Pongtawat from comment #26)
> I did some test by starting with phrases-icu.xml and change it towards
> words-icu.xml. Here is what I got:
> 
> 1. Adding the first two transliterate rules in words-icu.xml to
> phrases-icu.xml.
> --> Stack Smashed.
> 
> 2. Changing this line:
> 
> <transform rule="[:Punctuation:] Remove"/>
> 
> to
> 
> <transform rule="[[:WhiteSpace:][:Punctuation:]] Remove"/>
> 
> Now it index fine.
> 
> So, if I just add the WhiteSpace remove rule to phrases-icu.xml, what effect
> it will cause?

Hi,

in fact the conf of phrases-icu.xml is still very similar of words-icu.xml.
The conf is tested well on a Latin enviroment, with latin special char (è, à, Ł, Ń, etc.). So in a Thai enviroment, like you, probably is better to use a different configuration.

An help on setup is here: http://www.indexdata.com/yaz/doc/yaz-icu.html

In my opinion is good to have two files, one for words and one for phrase, but many  (particulary outsite Europe/America) need to personalize them.
Comment 30 Fridolin Somers 2014-06-04 10:43:04 UTC
Also have a look at : 
http://wiki.koha-community.org/wiki/Correcting_Search_of_Arabic_records
Comment 31 Colin Campbell 2014-06-06 08:15:21 UTC
(In reply to Fridolin SOMERS from comment #30)
> Also have a look at : 
> http://wiki.koha-community.org/wiki/Correcting_Search_of_Arabic_records

I'm not sure that all of the procedure outlined there is necessary. One of the sites we support has records in Arabic, Chinese and Cyrillic and tested the filing of each. Switching to ICU was all that was needed to get these records filing to their satisfaction. The transliterations in the wiki article were not required. Its an area that is unfortunately little documented
Comment 32 Kyle M Hall 2014-06-06 13:57:51 UTC
Pushed to 3.12.x will be in 3.12.13
Comment 33 Mason James 2015-02-25 13:27:10 UTC
(In reply to Galen Charlton from comment #23)
> Pushed to master.  Thanks, Colin!

Patch already included in 3.16.x branch