Bug 19581 - Elasticsearch - Catmandu split option adds extra null fields to indexes
Summary: Elasticsearch - Catmandu split option adds extra null fields to indexes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-05 01:50 UTC by Nick Clemens
Modified: 2018-05-22 10:59 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19581: Unit tests (2.30 KB, patch)
2017-11-05 02:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19581: Remove 'split' option from Catmandu fixes for marc (1.69 KB, patch)
2017-11-05 02:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19581: Unit tests (2.38 KB, patch)
2017-11-05 02:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19581: Remove 'split' option from Catmandu fixes for marc (1.75 KB, patch)
2017-11-10 20:32 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19581: Unit tests (2.44 KB, patch)
2017-11-10 20:32 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19581: Remove unused comment/variable (2.27 KB, patch)
2018-03-13 09:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19581: Remove unused comment and line (1.75 KB, patch)
2018-03-13 11:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19581: (follow-up) Fix handling of new and old arrays for auth compat (1.25 KB, patch)
2018-03-16 09:05 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19581: Remove 'split' option from Catmandu fixes for marc (1.82 KB, patch)
2018-04-09 12:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19581: Unit tests (2.51 KB, patch)
2018-04-09 12:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19581: Remove unused comment and line (1.82 KB, patch)
2018-04-09 12:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19581: (follow-up) Fix handling of new and old arrays for auth compat (1.32 KB, patch)
2018-04-09 12:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19581: Fix Koha_Elasticsearch.t (8.50 KB, patch)
2018-04-09 20:44 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 Nick Clemens 2017-11-05 01:50:03 UTC
This affects sorting, when encountering a null field the title in question will sort out of place.
Comment 1 Nick Clemens 2017-11-05 02:03:16 UTC
Created attachment 68947 [details] [review]
Bug 19581: Unit tests
Comment 2 Nick Clemens 2017-11-05 02:03:19 UTC
Created attachment 68948 [details] [review]
Bug 19581: Remove 'split' option from Catmandu fixes for marc

This option in combination with append seems to add null entries in the indexes
for fields that don't exist. These fields then affect sorting. Removing it solves
the issue

To test:
1 - Have an ES setup
2 - Catalog and index some records
3 - View a record in ES:
    curl 'localhost:9200/koha_kohadev_biblios/data/4?pretty=yes'
4 - Note there are 'null' entries in the indexes, save this result for
comparison
5 - Apply patch
6 - Reindex, amking sure to delete existing indexes
    perl misc/search_tools/rebuild_elastic_search.pl -d
7 - View the record again
8 - Note the null entries are removed
9 - Note indices are otherwise intact
Comment 3 Nick Clemens 2017-11-05 02:14:02 UTC
Created attachment 68949 [details] [review]
Bug 19581: Unit tests

This patch makes a few changes to the tests to take into account the
change to indexing.

To test:
prove t/db_dependent/Koha_Elasticsearch_Indexer.t
Should return green
Comment 4 David Bourgault 2017-11-10 20:32:24 UTC
Created attachment 69092 [details] [review]
Bug 19581: Remove 'split' option from Catmandu fixes for marc

This option in combination with append seems to add null entries in the indexes
for fields that don't exist. These fields then affect sorting. Removing it solves
the issue

To test:
1 - Have an ES setup
2 - Catalog and index some records
3 - View a record in ES:
    curl 'localhost:9200/koha_kohadev_biblios/data/4?pretty=yes'
4 - Note there are 'null' entries in the indexes, save this result for
comparison
5 - Apply patch
6 - Reindex, amking sure to delete existing indexes
    perl misc/search_tools/rebuild_elastic_search.pl -d
7 - View the record again
8 - Note the null entries are removed
9 - Note indices are otherwise intact

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 5 David Bourgault 2017-11-10 20:32:26 UTC
Created attachment 69093 [details] [review]
Bug 19581: Unit tests

This patch makes a few changes to the tests to take into account the
change to indexing.

To test:
prove t/db_dependent/Koha_Elasticsearch_Indexer.t
Should return green

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 6 David Bourgault 2017-11-10 20:34:12 UTC
QA tools green
Tests passed
Work as described. 

I could still values being set to null, but only as indicators of 00X fields, which I assume is on purpose.
Comment 7 Julian Maurice 2017-12-15 15:52:30 UTC
The code looks weird now, as we have

my $options = '';
...
$options = '' unless ...

So this line is completely useless.

Also, the comment above is about the split option which is not used anymore, can you update it please ?
Comment 8 Nick Clemens 2018-03-13 09:32:55 UTC
Created attachment 72745 [details] [review]
Bug 19581: Remove unused comment/variable
Comment 9 Nick Clemens 2018-03-13 11:20:07 UTC
Created attachment 72767 [details] [review]
Bug 19581: Remove unused comment and line
Comment 10 Nick Clemens 2018-03-16 09:05:23 UTC
Created attachment 72997 [details] [review]
Bug 19581: (follow-up) Fix handling of new and old arrays for auth compat

Stolen from bug 20244
Comment 11 Ere Maijala 2018-03-28 06:43:39 UTC
I think the join option would be useful (also in bug 20244), like this:

$options = "join:' '" unless $marc_field =~ m|_/| || $type eq 'sum';

Otherwise subfields of a single field get split to separate fields in ES.
Comment 12 Nick Clemens 2018-03-28 10:45:55 UTC
(In reply to Ere Maijala from comment #11)
> I think the join option would be useful (also in bug 20244), like this:
> 
> $options = "join:' '" unless $marc_field =~ m|_/| || $type eq 'sum';
> 
> Otherwise subfields of a single field get split to separate fields in ES.

I don't actually see a difference here - I added the line and reindexed and the record appeared the same, I tried changing title from 245a to 245 and 245abc - in all cases I got a single entry in the index - maybe I don't understand the issue?
Comment 13 Ere Maijala 2018-03-28 11:47:12 UTC
Thanks for checking. I'll try again to verify. It might have been a configuration issue on my end too.
Comment 14 Julian Maurice 2018-04-09 12:47:07 UTC
Created attachment 73864 [details] [review]
Bug 19581: Remove 'split' option from Catmandu fixes for marc

This option in combination with append seems to add null entries in the indexes
for fields that don't exist. These fields then affect sorting. Removing it solves
the issue

To test:
1 - Have an ES setup
2 - Catalog and index some records
3 - View a record in ES:
    curl 'localhost:9200/koha_kohadev_biblios/data/4?pretty=yes'
4 - Note there are 'null' entries in the indexes, save this result for
comparison
5 - Apply patch
6 - Reindex, amking sure to delete existing indexes
    perl misc/search_tools/rebuild_elastic_search.pl -d
7 - View the record again
8 - Note the null entries are removed
9 - Note indices are otherwise intact

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 15 Julian Maurice 2018-04-09 12:47:11 UTC
Created attachment 73865 [details] [review]
Bug 19581: Unit tests

This patch makes a few changes to the tests to take into account the
change to indexing.

To test:
prove t/db_dependent/Koha_Elasticsearch_Indexer.t
Should return green

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 16 Julian Maurice 2018-04-09 12:47:15 UTC
Created attachment 73866 [details] [review]
Bug 19581: Remove unused comment and line

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 17 Julian Maurice 2018-04-09 12:47:20 UTC
Created attachment 73867 [details] [review]
Bug 19581: (follow-up) Fix handling of new and old arrays for auth compat

Stolen from bug 20244

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 18 Jonathan Druart 2018-04-09 19:16:18 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 19 Jonathan Druart 2018-04-09 20:32:13 UTC
Lot of failures from t/db_dependent/Koha_Elasticsearch.t:

    #   Failed test at t/db_dependent/Koha_Elasticsearch.t line 153.
    #          got: 'marc_map('110a','author__sort.$append', )'
    #     expected: 'marc_map('110a','author__sort.$append', -split => 1)'
    # Looks like you failed 23 tests of 49.

Please fix ASAP

https://jenkins.koha-community.org/job/Koha_Master_D8/425/consoleFull
Comment 20 Jonathan Druart 2018-04-09 20:44:01 UTC
Created attachment 73896 [details] [review]
Bug 19581: Fix Koha_Elasticsearch.t
Comment 21 Jonathan Druart 2018-04-09 20:44:45 UTC
Ok done, pushed to master.
Comment 22 Nick Clemens 2018-05-14 21:13:25 UTC
Pushed to stable for 17.11.06

Awesome work all!
Comment 23 Fridolin SOMERS 2018-05-22 10:59:58 UTC
Not pushed to 17.05.x