Bug 17661 - Differences in field ending (whitespace, punctuation) cause duplicate facets
Summary: Differences in field ending (whitespace, punctuation) cause duplicate facets
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 31 votes (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
: 13231 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-11-18 16:41 UTC by Barton Chittenden
Modified: 2020-09-20 18:28 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04, 19.11.10


Attachments
490 $a Harry Bosch mysteries; $v 3 -- whitespace after semicolon in 490$a (1.08 KB, text/plain)
2016-11-18 16:41 UTC, Barton Chittenden
Details
490 $a Harry Bosch mysteries; $v 13 -- no space after semicolon (3.35 KB, text/plain)
2016-11-18 16:42 UTC, Barton Chittenden
Details
Bug 17661 - Ending punctuation causes duplicate facets (2.00 KB, patch)
2017-11-02 16:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17661 - Ending punctuation causes duplicate facets (4.30 KB, patch)
2020-07-07 16:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17661 - Ending punctuation causes duplicate facets (3.98 KB, patch)
2020-08-20 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17661 - Ending punctuation causes duplicate facets (4.05 KB, patch)
2020-08-25 15:46 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 17661: (follow-up) Update regex to support Unicode characters (2.49 KB, patch)
2020-08-27 16:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17661 - Ending punctuation causes duplicate facets (4.10 KB, patch)
2020-08-27 20:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17661: (follow-up) Update regex to support Unicode characters (2.54 KB, patch)
2020-08-27 20:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17661: Trivial simplification using += operator (942 bytes, patch)
2020-08-31 13:26 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2016-11-18 16:41:06 UTC
Created attachment 57658 [details]
490    $a Harry Bosch mysteries;  $v 3 -- whitespace after semicolon in 490$a

There are two marc files attached; 'a.marc' and 'b.marc'

Each contains the string "Harry Bosch mysteries;" in 490$a.

In a.marc, this is followed by a trailing space character, in b.marc, it is not.

If you import both records and do a keyword search for 'Harry Bosch mysteries', you will see that Harry Bosch mysteries shows twice in the Series facet.
Comment 1 Barton Chittenden 2016-11-18 16:42:11 UTC
Created attachment 57659 [details]
490    $a Harry Bosch mysteries; $v 13 -- no space after semicolon
Comment 2 Nick Clemens 2017-11-02 16:53:56 UTC
Created attachment 68897 [details] [review]
Bug 17661 - Ending punctuation causes duplicate facets

WIP
Comment 3 Nick Clemens 2017-11-02 16:54:27 UTC
Should expand this patch to cover whitespace, also need ES coverage, also better bug title
Comment 4 Sarah Cornell 2019-03-25 19:33:42 UTC
Correcting this display problem would really improve user experience. 

Currently users need to discover by trial and error that the two entries "Brunkus, Denise," and Brunkus, Denise." run the same search.  Our librarians assume they run different searches, and it makes them distrust the catalog.  

This affects all facets that come from MARC: authors, series, places, topics.
Comment 5 Tami Wilkerson 2019-03-25 20:03:14 UTC
(In reply to Sarah Cornell from comment #4)
> Correcting this display problem would really improve user experience. 
> 
> Currently users need to discover by trial and error that the two entries
> "Brunkus, Denise," and Brunkus, Denise." run the same search.  Our
> librarians assume they run different searches, and it makes them distrust
> the catalog.  
> 
> This affects all facets that come from MARC: authors, series, places, topics.

I completely agree with Sarah. The duplication pushes other authors, series, places, topics, etc. further down the list and users have to click "Show Me More" to see them.
Comment 6 Marjorie Barry-Vila 2019-04-05 15:41:04 UTC
+1
Comment 7 Katrin Fischer 2019-04-25 05:38:21 UTC
*** Bug 13231 has been marked as a duplicate of this bug. ***
Comment 8 kraborn 2019-07-30 14:18:36 UTC
My library would also be glad to see a resolution for this bug. The quality of our OPAC is part of the customer service we offer to our patrons. Misleading duplication in the OPAC because of how the 490 is read negatively impacts the patron experience rather than helping us make our OPAC as effective as possible. Thank you!
Comment 9 Katrin Fischer 2020-01-12 10:02:15 UTC
Hi Nick,

I thouhgt it would be very nice to revive this bug and looked at your WIP patch. I think you were on the right track there, but I'd like to suggest some changes to the regex.

You got:
$data =~ s/[.\-,]$//; 

I've tested with authors and made some changes on my data:

Without patch:

Aristophanes,
Aristophanes.
Beckett, Samuel,
Celan, Paul.
Date, C. J. 

With patch:

Aristophanes.
Beckett Samuel,
Celan Paul.
Date C. J. 

1) Your regex always removes the first occurence, but we need the last.
2) . is not removed as it's not escaped in the regex (I think).
2) In case of "Date, C. J." we don't want the last, when it's preceded by a capital letter (very likely to be an initial).
Comment 10 Nick Clemens 2020-07-07 16:45:32 UTC
Created attachment 106637 [details] [review]
Bug 17661 - Ending punctuation causes duplicate facets

To test:
1 - Do search and pull up some records
2 - Edit some of the records to have authors like:
    Date, C.J.
    Date, C.j.
    Date, C.J .
3 - Edit the records to have some series statments like:
    830 $aDate, C.J. ;$v5
    830 $aDate, C.J. ; $v5
    830 $aDate, C.J.; $v5
4 - Search again and note you have 3 facets each for authr and series
5 - Apply patch
6 - Repeat
7 - Now you get 2 facets for author, period not removed when following Upper case immediately, is otherwise
8 - Now you should have a single series facet
9 - Switch search engine to ES (index before applying patch)
10 - Note facets are separate again
11 - Reset mappings and reindex
   perl misc/search_tools/rebuild_elasticsearch -v -r
12 - Repeat search, facets combined as above
Comment 11 Nick Clemens 2020-07-07 16:47:53 UTC
(In reply to Katrin Fischer from comment #9)
> 1) Your regex always removes the first occurence, but we need the last.
> 2) . is not removed as it's not escaped in the regex (I think).
> 2) In case of "Date, C. J." we don't want the last, when it's preceded by a
> capital letter (very likely to be an initial).

1 - I had two different regexes, one was wrong
2 - period does not need to be escaped
3 - Done

I added eliminating spaces on either side of punctuation as well

Tested and played here to find cases:
https://regex101.com/

This is the base right now:
\s*(?<![A-Z])[.\-,;]\s*$

Let me know how that works
Comment 12 Nick Clemens 2020-08-20 15:03:07 UTC
Created attachment 108742 [details] [review]
Bug 17661 - Ending punctuation causes duplicate facets

The current code for facets doesn't pull strip ending punctuation from facets
This causes duplicate facets for terms that should be combined

Sometimes series can have different punctuation depending on the field they are in
Author initials punctuation should be preserved

To test:
1 - Do search and pull up some records
2 - Edit some of the records to have authors like:
    Date, C.J.
    Date, C.j.
    Date, C.J .
3 - Edit the records to have some series statments like:
    830 $aDate, C.J. ;$v5
    830 $aDate, C.J. ; $v5
    830 $aDate, C.J.; $v5
4 - Add some 490s to the record with first indicator 1 and series like:
    You wouldn't want to--
    You wouldn't want to
    You wouldn't want to..
5 - Search again and note you have 3 facets each for author and series
6 - Apply patch
7 - Repeat
8 - Now you get 2 facets for author, period not removed when following Upper case immediately, is otherwise
9 - Now you should have a single series facet
10 - Switch search engine to ES (index before applying patch)
11 - Note facets are separate again
12 - Reset mappings and reindex
   perl misc/search_tools/rebuild_elasticsearch -v -r
13 - Repeat search, facets combined as above
Comment 13 Andrew Fuerste-Henry 2020-08-25 15:46:13 UTC
Created attachment 109117 [details] [review]
Bug 17661 - Ending punctuation causes duplicate facets

The current code for facets doesn't pull strip ending punctuation from facets
This causes duplicate facets for terms that should be combined

Sometimes series can have different punctuation depending on the field they are in
Author initials punctuation should be preserved

To test:
1 - Do search and pull up some records
2 - Edit some of the records to have authors like:
    Date, C.J.
    Date, C.j.
    Date, C.J .
3 - Edit the records to have some series statments like:
    830 $aDate, C.J. ;$v5
    830 $aDate, C.J. ; $v5
    830 $aDate, C.J.; $v5
4 - Add some 490s to the record with first indicator 1 and series like:
    You wouldn't want to--
    You wouldn't want to
    You wouldn't want to..
5 - Search again and note you have 3 facets each for author and series
6 - Apply patch
7 - Repeat
8 - Now you get 2 facets for author, period not removed when following Upper case immediately, is otherwise
9 - Now you should have a single series facet
10 - Switch search engine to ES (index before applying patch)
11 - Note facets are separate again
12 - Reset mappings and reindex
   perl misc/search_tools/rebuild_elasticsearch -v -r
13 - Repeat search, facets combined as above

Signed-off-by: Sarah Cornell <sbcornell@cityofportsmouth.com>
Comment 14 Katrin Fischer 2020-08-26 21:51:07 UTC
This is a really great improvement and works really well with our sample data.

The only thing that I've been wondering about: seeing the A-Z as part of the regex: What will happen with non-latin and letters with diacritics? 

At the moment Ж. (Cyrillic) will lose the '.' and not be recognized as an initial. Is there a way we could make this work maybe? Could we check for the characters before being a space followed by a single character maybe?
Comment 15 Katrin Fischer 2020-08-26 22:07:50 UTC
I found some information here, that is promising:

https://perldoc.perl.org/perlunicode.html
For instance, \p{Uppercase} matches any single character with the Unicode "Uppercase" property, while \p{L} matches any character with a General_Category of "L" (letter) property 

So maybe we could just switch out [A-Z] for the \p{Uppercase}.
Comment 16 David Cook 2020-08-27 00:13:19 UTC
This is interesting.

I'm not surprised that "_get_facets_data_from_record" works in an undesirable way, but I'm surprised that this would be necessary for "_get_facet_from_result_set" using Zebra facets, since Zebra returns the data in a normalized format.

For example:

Z> elements zebra::facet::author:p
Z> show 1
Sent presentRequest (1+1).
Records: 1
Record type: XML
<record xmlns="http://www.indexdata.com/zebra/">
  <facet type="p" index="author">
    <term coccur="21" occur="11">berryman faye</term>
    <term coccur="11" occur="11">dale rae</term>
    <term coccur="11" occur="11">dale rae 1945</term>
    <term coccur="11" occur="11">fitzroy programs</term>
    <term coccur="12" occur="11">o carroll philip</term>
    <term coccur="10" occur="10">o carroll philip 1945</term>
    <term coccur="6" occur="6">powell jonathon</term>
    <term coccur="6" occur="6">powell jonathon illustrator</term>
    <term coccur="9" occur="6">reynolds kate e</term>
    <term coccur="12" occur="6">shapiro lawrence e</term>
    <term coccur="6" occur="5">carter emily</term>
    <term coccur="5" occur="3">caramagna joe</term>
    <term coccur="6" occur="3">schaefer charles e</term>
    <term coccur="4" occur="2">digeronimo theresa foy</term>
    <term coccur="4" occur="2">hughes edward e</term>
    <term coccur="4" occur="2">kazdin alan e</term>
    <term coccur="4" occur="2">reeve christine e</term>
    <term coccur="4" occur="2">renton n e nicholas edwin 1931</term>
    <term coccur="4" occur="2">snell martha e</term>
    <term coccur="4" occur="1">attwood tony</term>
  </facet>
</record>
nextResultSetPosition = 2
Elapsed: 0.030227

Now in this case there are some apparent "duplicates" where the author's date is included in some records but not others. I would argue that is an authority data issue though, since "John Smith", "John Smith 1945", and "John Smith 1995" are all different authors. 

I don't know how Elasticsearch handles its facets, so I can't comment there.
Comment 17 David Cook 2020-08-27 00:21:58 UTC
Yet, if I look at http://localhost:8081/cgi-bin/koha/catalogue/search.pl?q=test I'm not seeing data that looks like that facet data I showed before (which was from a different catalogue).

I'll dig a bit more...
Comment 18 David Cook 2020-08-27 00:23:10 UTC
Ah, that example was using CHR indexing.
Comment 19 David Cook 2020-08-27 00:31:16 UTC
Ok still using CHR indexing but using the unnormalized 0 register...

Previous results used "elements zebra::facet::author:p"

These results use "elements zebra::facet::au:0"

<record xmlns="http://www.indexdata.com/zebra/">
  <facet type="0" index="au">
    <term coccur="10" occur="10">O&apos;Carroll, Philip,</term>
    <term coccur="9" occur="9">Berryman, Faye</term>
    <term coccur="7" occur="7">Dale, Rae,</term>
    <term coccur="6" occur="6">Powell, Jonathon,</term>
    <term coccur="4" occur="4">Dale, Rae</term>
    <term coccur="4" occur="4">Shapiro, Lawrence E</term>
    <term coccur="3" occur="3">Reynolds, Kate E.,</term>
    <term coccur="2" occur="2">Anderson, Vicki</term>
    <term coccur="2" occur="2">Betts, Dion E.</term>
    <term coccur="2" occur="2">Brennan, Tom</term>
    <term coccur="2" occur="2">Caramagna, Joe</term>
    <term coccur="2" occur="2">Foxx, Richard M</term>
    <term coccur="2" occur="2">Gutstein, Steven E</term>
    <term coccur="2" occur="2">Medlen, Joan E. Guthrie.</term>
    <term coccur="2" occur="2">Renton, N. E. (Nicholas Edwin), 1931</term>
    <term coccur="2" occur="2">Reynolds, Kate E.</term>
    <term coccur="2" occur="2">Rosemann, Bill</term>
    <term coccur="2" occur="2">Schaefer, Charles E</term>
    <term coccur="2" occur="2">Snell, Martha E.</term>
    <term coccur="2" occur="1">Attwood, Tony</term>
  </facet>
</record>

I see that duplication now:
    <term coccur="7" occur="7">Dale, Rae,</term>
    <term coccur="4" occur="4">Dale, Rae</term>
Comment 20 David Cook 2020-08-27 00:34:24 UTC
One downside of post-processing facet results is that your facet list length will vary depending on its contents... 

It could be more reliable to handle this issue at index-time rather than search-time. 

Since we're using DOM indexing with Zebra, we have a lot of power at index-time (beyond the use of CHR and ICU mechanisms) which could handle whitespace and punctuation for facet purposes. 

That being said... post-processing is probably easiest to have the most consistent behaviour across 3 different facet providers...
Comment 21 Nick Clemens 2020-08-27 16:31:04 UTC
Created attachment 109242 [details] [review]
Bug 17661: (follow-up) Update regex to support Unicode characters

Rather than limiting initials to [A-Z] we should test for a broad
range of uppercase letters.

The ES/Zebra changes are slightly different because of Perl vs Java regex
conventions. POerl may support either, but I found 'Uppercase' to be a bit more explicit

More info here:
https://perldoc.perl.org/perlunicode.html

TO test:
Same plan as before but use Ж. as the ending initial
Confirm the period is preserved and other punctuation removed
Comment 22 Katrin Fischer 2020-08-27 20:02:20 UTC
Created attachment 109259 [details] [review]
Bug 17661 - Ending punctuation causes duplicate facets

The current code for facets doesn't pull strip ending punctuation from facets
This causes duplicate facets for terms that should be combined

Sometimes series can have different punctuation depending on the field they are in
Author initials punctuation should be preserved

To test:
1 - Do search and pull up some records
2 - Edit some of the records to have authors like:
    Date, C.J.
    Date, C.j.
    Date, C.J .
3 - Edit the records to have some series statments like:
    830 $aDate, C.J. ;$v5
    830 $aDate, C.J. ; $v5
    830 $aDate, C.J.; $v5
4 - Add some 490s to the record with first indicator 1 and series like:
    You wouldn't want to--
    You wouldn't want to
    You wouldn't want to..
5 - Search again and note you have 3 facets each for author and series
6 - Apply patch
7 - Repeat
8 - Now you get 2 facets for author, period not removed when following Upper case immediately, is otherwise
9 - Now you should have a single series facet
10 - Switch search engine to ES (index before applying patch)
11 - Note facets are separate again
12 - Reset mappings and reindex
   perl misc/search_tools/rebuild_elasticsearch -v -r
13 - Repeat search, facets combined as above

Signed-off-by: Sarah Cornell <sbcornell@cityofportsmouth.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2020-08-27 20:02:27 UTC
Created attachment 109260 [details] [review]
Bug 17661: (follow-up) Update regex to support Unicode characters

Rather than limiting initials to [A-Z] we should test for a broad
range of uppercase letters.

The ES/Zebra changes are slightly different because of Perl vs Java regex
conventions. POerl may support either, but I found 'Uppercase' to be a bit more explicit

More info here:
https://perldoc.perl.org/perlunicode.html

TO test:
Same plan as before but use Ж. as the ending initial
Confirm the period is preserved and other punctuation removed

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Jonathan Druart 2020-08-31 13:26:08 UTC
Created attachment 109368 [details] [review]
Bug 17661: Trivial simplification using += operator
Comment 25 Jonathan Druart 2020-08-31 14:20:19 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 26 Lucas Gass 2020-09-04 20:49:24 UTC
backported to 20.05.x for 20.05.04
Comment 27 Aleisha Amohia 2020-09-10 04:48:44 UTC
backported to 19.11.x for 19.11.10
Comment 28 Victor Grousset/tuxayo 2020-09-20 18:28:26 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.