With ES and QueryAutoTruncate on, a search with punctuation surrounded by spaces (like ISBD punctuation copied from other catalogue or bibliography) returns no results. E.g.: in a search for "Maria Stuart ; Die Jungfrau von Orleans / Schiller" (coded in record as 245 10 $a Maria Stuart ; $b Die Jungfrau von Orleans / $c Schiller) -- both semicolon and slash cause problems. One had to remove them manually to get results which is not what is desired.
Created attachment 164633 [details] [review] Bug 33407: With ES and QueryAutoTruncate on, a search containing ISBD punctuation returns no results With ES and QueryAutoTruncate on, a search with punctuation surrounded by spaces (like ISBD punctuation copied from other catalogue or bibliography) returns no results. E.g.: in a search for "Maria Stuart ; Die Jungfrau von Orleans / Schiller" (coded in record as 245 10 $a Maria Stuart ; $b Die Jungfrau von Orleans / $c Schiller) -- both semicolon and slash cause problems. One had to remove them manually to get results which is not what is desired. Test plan ========= 1. Use ktd with Elasticsearch and ktd's test data (http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=1): 2. Make a search for the first ISBD zone taken verbatim from the first record: E Street shuffle : the glory days of Bruce Springsteen & the E Street Band / Clinton Heylin There should be no result. 3. Apply teh patch, restart plack. 4. Repeat the search. You should get the record (and onother one). Sponsored-by: Ignatianum University in Cracow
Created attachment 164634 [details] [review] Bug 33407: Unit tests
Created attachment 164957 [details] [review] Bug 33407: With ES and QueryAutoTruncate on, a search containing ISBD punctuation returns no results With ES and QueryAutoTruncate on, a search with punctuation surrounded by spaces (like ISBD punctuation copied from other catalogue or bibliography) returns no results. E.g.: in a search for "Maria Stuart ; Die Jungfrau von Orleans / Schiller" (coded in record as 245 10 $a Maria Stuart ; $b Die Jungfrau von Orleans / $c Schiller) -- both semicolon and slash cause problems. One had to remove them manually to get results which is not what is desired. Test plan ========= 1. Use ktd with Elasticsearch and ktd's test data (http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=1): 2. Make a search for the first ISBD zone taken verbatim from the first record: E Street shuffle : the glory days of Bruce Springsteen & the E Street Band / Clinton Heylin There should be no result. 3. Apply teh patch, restart plack. 4. Repeat the search. You should get the record (and onother one). Sponsored-by: Ignatianum University in Cracow Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 164958 [details] [review] Bug 33407: Unit tests Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
The patch does not apply anymore because the number of tests seem to have changed. The fix should be trivial.
Created attachment 167410 [details] [review] Bug 33407: Unit tests Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
(In reply to Thomas Klausner from comment #5) > The patch does not apply anymore because the number of tests seem to have > changed. > > The fix should be trivial. Thanks, Thomas. Please, try it now.
Created attachment 167419 [details] [review] Bug 33407: With ES and QueryAutoTruncate on, a search containing ISBD punctuation returns no results removed spurious newline from patch. Hope this works...
Created attachment 167420 [details] [review] Bug 33407: Unit tests
Works as advertised. I took a quick look at the added regex and .. wow. An escaped escaped slash :-) But it works, so: PASS QA! (my first!) PS: I removed a spurious newline in the code patch, and therefore had to re-upload both patches. I hope this is OK...
Minor QA point.. you forgot to add your signoff line to the patches Thomas. I've added them to the patch I'm about to push however, so no harm done.
Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant
I think that this might break searches where people are relying on the punctuation to be included. Feels a bit like we're repeating mistakes we made with Zebra in the past...
(In reply to David Cook from comment #13) > I think that this might break searches where people are relying on the > punctuation to be included. > > Feels a bit like we're repeating mistakes we made with Zebra in the past... This is only removing punctuation surrounded by spaces, if the terms are quoted, it won't be removed. It is difficult here to balance between exact results and meeting user expectations. I worry we are getting a bit confusing as we massage the queries more, but we still have user complaints about the over exactness of some features in ES Looking over this one again, it seems the problem only does occur when QueryAutoTruncate is enabled - so possibly the regex should be surrounded by a checked for this pref? That would minimize the scope of the 'magic' here
(In reply to Nick Clemens (kidclamp) from comment #14) > This is only removing punctuation surrounded by spaces, if the terms are > quoted, it won't be removed. We don't have unit tests that confirm that. Only one test was added for one particular scenario. Note that this query string isn't tested without QueryAutoTruncate in the unit tests... > It is difficult here to balance between exact results and meeting user > expectations. I worry we are getting a bit confusing as we massage the > queries more, but we still have user complaints about the over exactness of > some features in ES I am concerned about the massaging of queries. Could you give some examples of the over exactness? I have found it interesting needing to do 'local-number:"1"' instead of 'local-number:1' but I've adapted. > Looking over this one again, it seems the problem only does occur when > QueryAutoTruncate is enabled - so possibly the regex should be surrounded by > a checked for this pref? That would minimize the scope of the 'magic' here It looks like the $truncate variable there is set by QueryAutoTruncate. But since QueryAutoTruncate is on out of the box, we can think of this as default behaviour. It can be tough to change things away from defaults. -- Our main issue with Elasticsearch is timeouts/inconsistent indexing and not a lack of compatibility in a number of areas (like date searching). I can't say wildcards/truncation has really been an issue. It's interesting though. Much left to investigate and improve...
Created attachment 167649 [details] [review] Bug 33407: (follow-up) Expand tests This expands the tests to cover all removed punctuation and confirm that it is not removed when not surrounded by spaces. It also shows that repeated punctuation is removed. I cannot make the slashes match in any case - can we get a follow up to prove that case Janusz?
(In reply to David Cook from comment #15) > (In reply to Nick Clemens (kidclamp) from comment #14) > > This is only removing punctuation surrounded by spaces, if the terms are > > quoted, it won't be removed. > > We don't have unit tests that confirm that. Only one test was added for one > particular scenario. > > Note that this query string isn't tested without QueryAutoTruncate in the > unit tests... I added more tests for when truncate is enabled - it would be good to repeat the tests without the syspref - can we get a follow-up Janusz? > Could you give some examples of the over exactness? I have found it > interesting needing to do 'local-number:"1"' instead of 'local-number:1' but > I've adapted. The above should be fixed by bug 32707. In general, Zebra was more forgiving: without truncation 'idea' would match 'ideas' or 'ideation' 'blues' would match 'blue' Punctuation too: dont/don't matched both ways > It looks like the $truncate variable there is set by QueryAutoTruncate. > > But since QueryAutoTruncate is on out of the box, we can think of this as > default behaviour. It can be tough to change things away from defaults. Oops, I forgot the conditional, thanks :-) > Our main issue with Elasticsearch is timeouts/inconsistent indexing and not > a lack of compatibility in a number of areas (like date searching). I can't > say wildcards/truncation has really been an issue. It's interesting though. > Much left to investigate and improve... Hmm...we've done a number of enhancements to the indexer to improve this and ensure smaller batches to avoid timeouts - could you open a bug with more details?
(In reply to Nick Clemens (kidclamp) from comment #17) > In general, Zebra was more forgiving: > without truncation 'idea' would match 'ideas' or 'ideation' > 'blues' would match 'blue' That would be due to the QueryStemming syspref I think. > Punctuation too: dont/don't matched both ways That was due to custom CHR/ICU indexing rules on the server-side, and that also solved some problems while causing others. > > Our main issue with Elasticsearch is timeouts/inconsistent indexing and not > > a lack of compatibility in a number of areas (like date searching). I can't > > say wildcards/truncation has really been an issue. It's interesting though. > > Much left to investigate and improve... > > Hmm...we've done a number of enhancements to the indexer to improve this and > ensure smaller batches to avoid timeouts - could you open a bug with more > details? I think that I've figured that one out now. It looks like Azure doesn't let idle persistent TCP connections last longer than 4 minutes, and it's been silently dropping connections. I've made some server tweaks and I'm in a monitoring stage at the moment, but planning to write more on the problem and solution soon, if all goes the way I think it is going. (Probably a wiki entry and I'll email the listservs about it.)
(In reply to David Cook from comment #18) > (In reply to Nick Clemens (kidclamp) from comment #17) > > In general, Zebra was more forgiving: > > without truncation 'idea' would match 'ideas' or 'ideation' > > 'blues' would match 'blue' > > That would be due to the QueryStemming syspref I think. Actually, even with the stemming, that would've still needed truncation. I'm a bit skeptical about this one, but open to hearing more.
(In reply to David Cook from comment #19) > (In reply to David Cook from comment #18) > > (In reply to Nick Clemens (kidclamp) from comment #17) > > > In general, Zebra was more forgiving: > > > without truncation 'idea' would match 'ideas' or 'ideation' > > > 'blues' would match 'blue' > > > > That would be due to the QueryStemming syspref I think. > > Actually, even with the stemming, that would've still needed truncation. I'm > a bit skeptical about this one, but open to hearing more. We should probably open a new bug to continue discussion :-) My tests still need a confirmation/follow-up from Janusz
Not going to backport this one until the follow-ups/additional work are QA'ed.