https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #2 from David Gustafsson <glasklas@gmail.com> --- Created attachment 70200 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70200&action=edit patched -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #3 from David Gustafsson <glasklas@gmail.com> --- Created attachment 70201 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70201&action=edit Bug 19893 - Alternative optimized indexing for Elasticsearch Add alternative optimized indexing for Elasticsearch How to test: 1) Time a full elasticsearch re-index by running the rebuild_elastic_search.pl with the -d flag: `koha-shell <instance_name> -c "time rebuild_elastic_search.pl -d"`. 2) Enable ExperimentalElasticSearchIndexing system preference (found under Global System preferences -> Administration -> Search Engine). 3) Time a full re-index again, it should be about twice at fast (for a couple of thousand biblios, with fewer biblios results may be more unpredictable). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- Fixed an instance of hard-coded Elasticsearch mapping document type in update_mappings(). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #8 from David Gustafsson <glasklas@gmail.com> --- Fixed decoding of marc records returned from Elasticsearch (depending on setting) in some places where this was missing. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #10 from David Gustafsson <glasklas@gmail.com> --- Wrong issue, sorry. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #12 from David Gustafsson <glasklas@gmail.com> --- Ok, have not tried it for unimarc-records myself, but from the error messages it appears the perl MARC-library expects subfield 100a to contain encoding information, and that it's emtpy. Right now not really sure if this is a MARC:::File::XML issue, data issue, or a bug in the patch. It could perhaps be worked around if added option for which serialization format to use (binary marc in addition to marc-xml). Opted for marc-xml mainly because of the length-limitation, we have some records with too many items so will not fit into binary marc. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #14 from David Gustafsson <glasklas@gmail.com> --- Fixed an unrelated issue where rebuild-index crashed if index did not already exist, will now create index if not exists. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20244 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20244 [Bug 20244] Elasticsearch - Indexing improvements -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #17 from David Gustafsson <glasklas@gmail.com> --- Thanks

1.) Incomplete fields should be handled more gracefully...

I agree. Have ignored this since only results in a warning, but could probably be done in better way. Some mappings ranges in the mappings YAML are off by one (mostly the ff*-fields I think), this might aggravate the issue.

2.) es_id is missing from the indexed records compared to the Catmandu indexing code. Is this intentional?

I think this is something Catmandu adds, and I think I just left it out since doesn't seems to be needed for anything.

3.) I think this, when done, should just replace the Catmandu-based indexing code. Since the ES support itself is somewhat experimental, it would make sense to switch once and for all.

That would be wonderful. I would actually love to get rid of all Catmandu-dependencies altogether. Indexing is probably the heaviest part, the rest should be quite trivial to replace with Search::Elasticsearch (which Catamndu uses internally).

4.) Make sure to document the dependency on Search::Elasticsearch. Would it be possible to use the v6 module? It says it supports ES 5 too. I had to downgrade it on my system for the patch to work.

I did not document it since Catmandu depends on it, but it will need to be done if Catmandu is no longer a dependency. To use v6 you just have to change, client => "5_0::Direct" to client => "6_0::Direct". If I'm not mistaken, I think we are running ES 6 with the "5_0::Direct" line (and works, but probably not optimal). The reason I went for 5.0 was that was the version Koha was using at the time of the initial version of the patch (I think).

5.) Booleans should be indexed as true/false. I'm seeing deprecation notices for suppress and onloan (this is wrong in the old code too, but could as well be done correctly here).

I agree. This is a problem in Koha master as well I think (but there is a bugzilla issue that takes care of it). It would be easy to fix so will make sure to do so.

6.) The Catmandu version seems to create way more __sort fields, but perhaps it's a bug in the Catmandu version?

Yes, I think this is a bug in Koha master, but will look into to it.

7.) The patch doesn't apply cleanly, so there could be also something I screwed up while fixing it manually.

I think you got it right, most of the above I have run into myself, though a bit strange that you had to downgrade to 5.0 to get it to work? Can verify tomorrow that it really is 6.x we are running. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #18 from David Gustafsson <glasklas@gmail.com> --- About the version, I was wrong, seams we are running 5.4. I would assume it would be as easy to just change the version string to get it working with 6 (but not sure how 6 handles _all fields and other deprecated things used by Koha). We run a quite heavily patched version of Koha where those issues have been fixed. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #20 from Ere Maijala <ere.maijala@helsinki.fi> --- Actually, now that 20073 is in master, the "include_in_all" issue with ES 6 should be resolved. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #22 from David Gustafsson <glasklas@gmail.com> --- Rebased against master, have not had time to address the other issues, but will do so as soon as I can. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #24 from David Gustafsson <glasklas@gmail.com> --- Fixed the offset issue (which I should have done sooner as was a very minor fix). Search::Elasticsearch had been added to Perl dependencies somewhere else and was included with the rebase. es_id not needed as far as I can tell. Fixed boolean deprication warning and introduced "value_callback" mapping option which can later be made more dynamic, for example let type implementations define callbacks instead of hard coded in get_marc_mapping_rules. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #75271|0 |1 is obsolete| | --- Comment #25 from David Gustafsson <glasklas@gmail.com> --- Created attachment 75272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75272&action=edit Bug 19893 - Alternative optimized indexing for Elasticsearch Add alternative optimized indexing for Elasticsearch How to test: 1) Time a full elasticsearch re-index by running the rebuild_elastic_search.pl with the -d flag: `koha-shell <instance_name> -c "time rebuild_elastic_search.pl -d"`. 2) Enable ExperimentalElasticSearchIndexing system preference (found under Global System preferences -> Administration -> Search Engine). 3) Time a full re-index again, it should be about twice at fast (for a couple of thousand biblios, with fewer biblios results may be more unpredictable). Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #27 from David Gustafsson <glasklas@gmail.com> --- (In reply to Nicolas Legrand from comment #11)

It breaks my authority indexing (Unimarc) :

substr outside of string at /home/koha/perl5/lib/perl5/MARC/File/XML.pm line 563. Use of uninitialized value $enc in string eq at /home/koha/perl5/lib/perl5/MARC/File/XML.pm line 565. Use of uninitialized value $enc in string eq at /home/koha/perl5/lib/perl5/MARC/File/XML.pm line 565. Use of uninitialized value $enc in string eq at /home/koha/perl5/lib/perl5/MARC/File/XML.pm line 567. Use of uninitialized value $enc in concatenation (.) or string at /home/koha/perl5/lib/perl5/MARC/File/XML.pm line 570. Unsupported UNIMARC character encoding [] for XML output for unimarc; 100$a -> 20180313 frey50 at /home/koha/perl5/lib/perl5/MARC/File/XML.pm line 570.

I had forgotten about this issue, I will try to look into it, it would be easier if I had some test data which produced this error. So if you have a marc file would be helpful to attach or mail it to glasklas@gmail.com. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #75272|0 |1 is obsolete| | --- Comment #29 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 75353 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75353&action=edit Bug 19893 - Alternative optimized indexing for Elasticsearch Add alternative optimized indexing for Elasticsearch How to test: 1) Time a full elasticsearch re-index by running the rebuild_elastic_search.pl with the -d flag: `koha-shell <instance_name> -c "time rebuild_elastic_search.pl -d"`. 2) Enable ExperimentalElasticSearchIndexing system preference (found under Global System preferences -> Administration -> Search Engine). 3) Time a full re-index again, it should be about twice at fast (for a couple of thousand biblios, with fewer biblios results may be more unpredictable). Sponsored-by: Gothenburg University Library Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #31 from David Gustafsson <glasklas@gmail.com> --- Ok, nice! Are you referring to adding Search::Elasticsearch in PerlDependencies? It has already been added it seams, so don't need to handle that dependency. If it is deemed the best option I can refactor the patch to remove the preference and use this as the only indexing option, that would not take long. Would be nice with some more feedback regarding how to handle this. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #33 from David Gustafsson <glasklas@gmail.com> --- Yes, you are probably right. Initially I provided it as a preference since was more uncertain if stable or not and did not want to cause regressions when the current indexing was working. But now I'm a lot more confident that at least all serious issues have been ironed out, and we have been running this in production with ~2 million biblios since the beginning of April. As you say as Elastic is not stable yet it's probably a good to replace indexing logic with this so that possible issues can be caught by more people (the possible unimarc issue for example). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #35 from David Gustafsson <glasklas@gmail.com> --- No problem, I can do that, what should that mail contain? In the first comment I described the main motivation behind the patch, should I write more about the technical details how the code works? Also, even with this patch Catmandu is still used for searching, but that part I think it will be easy to replace with Search::ElasticSearch client to be able to drop Catmandu-dependency completely. Since this patch in it's current state seams pretty stable, it would perhaps be better to do this in a separate issue later if this one gets merged. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #37 from Nicolas Legrand <nicolas.legrand@bulac.fr> --- (In reply to David Gustafsson from comment #27) [...]

I had forgotten about this issue, I will try to look into it, it would be easier if I had some test data which produced this error. So if you have a marc file would be helpful to attach or mail it to glasklas@gmail.com.

Hey David, my colleague Séverine Queune retested it, it worked well this time and it took 30% less time than usual! We're not well verse on how it should work, but so far, having something that works and works faster seems pretty convenient :). Thanks! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Mirko Tietgen <mirko@abunchofthings.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mirko@abunchofthings.net --- Comment #39 from Mirko Tietgen <mirko@abunchofthings.net> --- Could you please list the versions of catmandu modules you did the tests with? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #75353|0 |1 is obsolete| | --- Comment #40 from David Gustafsson <glasklas@gmail.com> --- Created attachment 75428 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75428&action=edit Bug 19893 - Alternative optimized indexing for Elasticsearch Add alternative optimized indexing for Elasticsearch How to test: 1) Time a full elasticsearch re-index by running the rebuild_elastic_search.pl with the -d flag: `koha-shell <instance_name> -c "time rebuild_elastic_search.pl -d"`. 2) Enable ExperimentalElasticSearchIndexing system preference (found under Global System preferences -> Administration -> Search Engine). 3) Time a full re-index again, it should be about twice at fast (for a couple of thousand biblios, with fewer biblios results may be more unpredictable). Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #42 from David Gustafsson <glasklas@gmail.com> --- Forget to mention that in the new version of the patch I removed the preference and the new indexing method instead replaces the old. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #44 from David Gustafsson <glasklas@gmail.com> --- Ok! I think these were the versions: libcatmandu-marc-perl 0.215-1 libcatmandu-perl 1.0304-2~kohadev1 libcatmandu-store-elasticsearch-perl 0.0507-1~kohadev1 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #46 from David Gustafsson <glasklas@gmail.com> --- Added preference to choose serialization format for records in index. Now providing also base64 encoded binary marc as a more performant but possibly less robust (will not support very large records) option. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #48 from David Gustafsson <glasklas@gmail.com> --- Ok! MARCXML is still the default format, so previous behaviour is maintained if preference is left as is. The performance difference was also as big as hoped on the whole, so the largest benefit might be smaller index. I could remove it though quite confident it does not introduces any new bugs. Also I accidentally removed you sign off, but since have changed some things since then perhaps best to sign off again in that case. What I should have done is add the new changes as new commits, but wanted a clean diff to make it easier to review as a single unit, but in this case new commits would probably have been better. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #51 from Nick Clemens <nick@bywatersolutions.com> --- *** Bug 20768 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #53 from Ere Maijala <ere.maijala@helsinki.fi> --- David, $serialization_format needs a default value in the code, I think (but see below). Otherwise, unless you go and save the ElasticsearchMARCSerializationFormat pref once, the search results show up empty and you'll get a lot of "uninitialized value" warnings from the following line: if ($serialization_format eq 'base64ISO2709') { I also believe that "$record->as_usmarc()" will carp and set the record length to 99999 if the record ends up being too long. If you made reading the index autodetect between ISO2709 and MARCXML, you could use MARCXML as the fallback format when ->as_usmarc() carps. So the ISO2709 format would be used whenever possible and MARCXML would only be used for large records. And you could get rid of the ElasticsearchMARCSerializationFormat pref. This would be my preferred method at the moment, since ISO2709 saves time and space but large records need to be supported. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #54 from David Gustafsson <glasklas@gmail.com> --- Thanks for the review and good suggestions. I agree the hybrid method seems the best, could also save a marc_format so format does not have to be detected. I have also considered the two option you mentioned, storing the whole marc record as json or skip storing it all together. The later could possibly be an option (for those wanting to keep index size small), the former would be really and would like to see implemented so could be the subject for a future bug. The catamandu json serialization format is pretty useless for querying and analysis, but it probably not take that much work to write a marc to json direct conversion which allowed you to make queries like 100.a:"Tolkien, J. R. R.". I would also like to use Kibana for analysis and for that to be useful you would have t have the marc data available as structured json. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #56 from Mirko Tietgen <mirko@abunchofthings.net> --- (In reply to Nick Clemens from comment #34)

Can you send something to the koha-dev mailing list and add this as a topic for the next Developers meeting David?

In a situation like this we should check for differing opinions or see if there are concerns others might have, and then we can vote at the meeting to move ahead.

I think this the way to go, one less dependency makes it easier to maintain

I am not necessarily a fan of dropping dependencies. I think it is part of Perl to use other modules instead of reinventing everything. Regarding Catmandu, the developers are very dedicated and we will profit from future developments without investing Koha developer time, while a change to our own solution might help with performance now but might not be touched again for a long time. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #59 from David Gustafsson <glasklas@gmail.com> --- And with what this is the latest versions: (catmandu 1.09, catmandu-marc 1.231, catmandu-store-elasticsearch 0.0507) real 1m59.575s user 1m6.496s sys 0m3.252s Also a correction catmandu-marc 0.09 in the previous reply should of course be 1.09. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #61 from David Gustafsson <glasklas@gmail.com> --- Forgot to mention that the timings where taking using about 5000 records and 277 maps. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #63 from David Gustafsson <glasklas@gmail.com> --- Created attachment 75658 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75658&action=edit Bug 19893: Add index status Add persistent per index "index status" state to provide useful user feedback when update of Elasticsearch server mappings fails -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #75658|0 |1 is obsolete| | --- Comment #65 from David Gustafsson <glasklas@gmail.com> --- Created attachment 75660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75660&action=edit Bug 19893: Add index status Add persistent per index "index status" state to provide useful user feedback when update of Elasticsearch server mappings fails -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #67 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Have we come to a conclusion here? What would be the next step? Should we add it to the agenda for next dev's meeting? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #69 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Bug 20073 is now in.. I like the look of this one and will devote a bit of time to help QA it if we get a signoff soon. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #75455|0 |1 is obsolete| | --- Comment #70 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 76612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76612&action=edit Bug 19893 - Alternative optimized indexing for Elasticsearch Implement optimized indexing for Elasticsearch How to test: 1) Time a full elasticsearch re-index without this patch by running the rebuild_elastic_search.pl with the -d flag: `koha-shell <instance_name> -c "time rebuild_elastic_search.pl -d"`. 2) Apply this patch. 3) Time a full re-index again, it should be about twice at fast (for a couple of thousand biblios, with fewer biblios results may be more unpredictable). Sponsored-by: Gothenburg University Library Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #75660|0 |1 is obsolete| | --- Comment #72 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 76614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76614&action=edit Bug 19893: Add index status Add persistent per index "index status" state to provide useful user feedback when update of Elasticsearch server mappings fails Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #74 from David Gustafsson <glasklas@gmail.com> --- Ok, great! Thanks for the sign-off. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #76 from Ere Maijala <ere.maijala@helsinki.fi> --- David, Martin: Please also check out some further fixes (built on top of the work done here) in bug 20244. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #77 from Ere Maijala <ere.maijala@helsinki.fi> --- David, the current patch set doesn't apply cleanly. Could you rebase? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #79 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 78092 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78092&action=edit Bug 19893: Remove serialization format setting Default to base64 encoded binary MARC with MARCXML fallback if record exceeds maximum size Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #81 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 78094 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78094&action=edit Fix Elasticsearch tests. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Alex Arnaud <alex.arnaud@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |alex.arnaud@biblibre.com --- Comment #83 from Alex Arnaud <alex.arnaud@biblibre.com> --- I try to check the difference between the data indexed in ES before and after this patch. First, concatenation seems to no longer work. I have the following mapping author => 700ba #($b = firstname, $a = lastname). With Catendu::* libraries, i can read this in ES database: "author" : [ "Hanna Johansen", "JohansenHanna", "MaquetYves-Marie", "BehndKathi" ], (There is sometime no space but it's an other problem) With Koha-specific code: "author" : [ "Hanna Johansen", "Johansen", "Hanna", "Maquet", "Yves-Marie", "Behnd", "Kathi" ], Also, there is some difference on the data structure: Catmendu::* : "author__suggestion" : [ { "input" : "Hanna Johansen" }, { "input" : "Johansen" }, { "input" : "Hanna" } ], Koha-specific: "author__suggestion" : { "input" : [ "Hanna Johansen", "JohansenHanna" ] }, Is this wanted ? What are the consequences? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |ASSIGNED --- Comment #84 from Ere Maijala <ere.maijala@helsinki.fi> --- I don't think that's wanted. It just slipped by. I have some improvements in bug 20244, but the relevant parts should probably be moved here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #86 from David Gustafsson <glasklas@gmail.com> --- And to clarify: I meant concatenation without any space in between will cause issues with tokenization, bug 20244 joins with space as separator so not an issue. I still don't understand why though :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #88 from David Gustafsson <glasklas@gmail.com> --- Yes, I'm aware of that. The problem as I see it is that subfields don't have any defined order in which the information becomes meaningful as a sentence. So proximity matching could result in false positives in this case, if I'm not missing something. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Sally Healey <sally.healey@cheshiresharedservices.gov.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sally.healey@cheshireshared | |services.gov.uk -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #91 from Ere Maijala <ere.maijala@helsinki.fi> --- If the fields don't belong together, you can always define them alone as is currently done in many cases. It's just that you should also be able to say that you want 245ab as a whole. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #93 from Ere Maijala <ere.maijala@helsinki.fi> --- (In reply to Katrin Fischer from comment #92)

(In reply to Ere Maijala from comment #89)

...

But they do. E.g. 245$a and $b form the full title.

I'd add $n and $p :)

Sure :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #95 from David Gustafsson <glasklas@gmail.com> --- (In reply to Ere Maijala from comment #91)

If the fields don't belong together, you can always define them alone as is currently done in many cases. It's just that you should also be able to say that you want 245ab as a whole.

Sure, I can buy that. But that is not the way the mappings are currently defined in most places. For example Personal-name is mapped to "100abcdefghjklmnopqrstvxyz" and would have to be split up into one mapping for each subfield, like 100a, 100b, 100c, 100d etc, or all the information will be join into a string that does not make much sense. This will make search behavior harder to predict, potentially cause unexpected exact/proximity matches and thus more difficult to optimize. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #97 from David Gustafsson <glasklas@gmail.com> --- (In reply to David Gustafsson from comment #95)

(In reply to Ere Maijala from comment #91)

...

If the fields don't belong together, you can always define them alone as is currently done in many cases. It's just that you should also be able to say that you want 245ab as a whole.

Sure, I can buy that. But that is not the way the mappings are currently defined in most places. For example Personal-name is mapped to "100abcdefghjklmnopqrstvxyz" and would have to be split up into one mapping for each subfield, like 100a, 100b, 100c, 100d etc, or all the information will be join into a string that does not make much sense. This will make search behavior harder to predict, potentially cause unexpected exact/proximity matches and thus more difficult to optimize.

I just realized that splitting up the fields would not make any difference with the Koha-specific implementation, it makes no distinction if subfield mappings are defined together in one mapping or separately. I would guess that it would be much more efficient to introduce concaternation as an exception, with special syntax (as there appears to be only a few mappings that have use for it), and keep default behavior as it is. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #99 from Alex Arnaud <alex.arnaud@biblibre.com> --- (In reply to David Gustafsson from comment #96)

@Alex Arnaud I agree concaternation of subfields could be useful, but I ague that how this is done has to be defined in the mappings. If we concatenate all the subfields, many existing fields where concatenation is not desirable will be concaternated as a result.

I think the mapping system was designed for that. I.e author: 700ba # we get 700$a and 700$b concatenated And when concatenation is not wanted: author: 700b author: 700a # We get 700$a and 700$b in 2 different array elements -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #101 from David Gustafsson <glasklas@gmail.com> --- I think some tests got removed (don't know if by mistake or not), but restored them and adjusted for the new return values introduced with the patch. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #103 from Nicolas Legrand <nicolas.legrand@bulac.fr> --- (In reply to David Gustafsson from comment #102) [...]

...

I think the mapping system was designed for that. I.e

author: 700ba # we get 700$a and 700$b concatenated

And when concatenation is not wanted:

author: 700b author: 700a # We get 700$a and 700$b in 2 different array elements

In that case there are MANY mappings that are defined in the wrong way.

True, but we recently learned the hard way we need those kind of concatenation in the zone 7xx. We have cases of "authorfirstname authorlastname" that don't work and should. Rewriting mappings is not expansive compare to this. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #105 from Alex Arnaud <alex.arnaud@biblibre.com> --- (In reply to Nicolas Legrand from comment #103)

(In reply to David Gustafsson from comment #102) [...]

...

...

I think the mapping system was designed for that. I.e

author: 700ba # we get 700$a and 700$b concatenated

And when concatenation is not wanted:

author: 700b author: 700a # We get 700$a and 700$b in 2 different array elements

In that case there are MANY mappings that are defined in the wrong way.

True, but we recently learned the hard way we need those kind of concatenation in the zone 7xx. We have cases of "authorfirstname authorlastname" that don't work and should. Rewriting mappings is not expansive compare to this.

Hello Nicolas, Any exemples of what doesn't work ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #107 from Séverine Queune <severine.queune@bulac.fr> --- (In reply to Alex Arnaud from comment #105)

(In reply to Nicolas Legrand from comment #103)

...

(In reply to David Gustafsson from comment #102) [...]

...

...

I think the mapping system was designed for that. I.e

author: 700ba # we get 700$a and 700$b concatenated

And when concatenation is not wanted:

author: 700b author: 700a # We get 700$a and 700$b in 2 different array elements

In that case there are MANY mappings that are defined in the wrong way.

True, but we recently learned the hard way we need those kind of concatenation in the zone 7xx. We have cases of "authorfirstname authorlastname" that don't work and should. Rewriting mappings is not expansive compare to this.

Hello Nicolas,

Any exemples of what doesn't work ?

Hi Alex, On Bulac catalog, you can search for "Ariane Eissen". You'll have 2 results, but we possess 3 books she wrote. In the third record, "Ariane" is preceded by "sous la direcetion d' " in 200$f so with the apostophe, the name of the author is considered as "d'Ariane". This is quite an obscur problem to us, because zone 7xxx is correctly indexed and authorities are present, so we could have found this third record using both "Ariane" and "d'Ariane". I just test with the form you mentionned on comment 94 and it works : I've got 3 results searching "eissenariane". Could that exemple help you ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #109 from David Gustafsson <glasklas@gmail.com> --- And I would not say that concaternation currently works if the result is, "JohansenHanna", "MaquetYves-Marie" or "BehndKathi" for example (missing space). Probably easy to correct, but could be improved further with the optimized indexing as mentioned above. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #111 from David Gustafsson <glasklas@gmail.com> --- Created attachment 78561 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78561&action=edit Bug 19893: Support for joined subfields in mappings -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #113 from Ere Maijala <ere.maijala@helsinki.fi> --- Is there a real use case for not adding the space? Obviously it's not needed if it's there already, but that can be done automatically. I'm asking because conditional logic here gets a bit more complicated when you take optional subfields into consideration. What I mean is that the rule marc_field: 245(ab)cdef is clear when both a and b exist, but what if there's only acd? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #115 from Nicolas Legrand <nicolas.legrand@bulac.fr> --- (In reply to Alex Arnaud from comment #105)

(In reply to Nicolas Legrand from comment #103)

...

(In reply to David Gustafsson from comment #102) [...]

...

...

I think the mapping system was designed for that. I.e

author: 700ba # we get 700$a and 700$b concatenated

And when concatenation is not wanted:

author: 700b author: 700a # We get 700$a and 700$b in 2 different array elements

In that case there are MANY mappings that are defined in the wrong way.

True, but we recently learned the hard way we need those kind of concatenation in the zone 7xx. We have cases of "authorfirstname authorlastname" that don't work and should. Rewriting mappings is not expansive compare to this.

Hello Nicolas,

Any exemples of what doesn't work ?

Hey Alex https://koha.bulac.fr/cgi-bin/koha/opac-MARCdetail.pl?biblionumber=914403 Try to find this book while taping the name of the co-author "Ariane Eissen". Her name appears fully in the field 200, but the lexer don't separate words on apostrophes, so she's indexed as "d'Ariane Eissen". This leave us with the 702 to find her. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #117 from Nicolas Legrand <nicolas.legrand@bulac.fr> --- (In reply to Ere Maijala from comment #116)

The apostrophe is a separate issue and should be handled by modifying the analysis chain config in Elasticsearch. I haven't tested it yet, but ES has an elision token filter for just this purpose: https://www.elastic.co/guide/en/elasticsearch/reference/current/analysis- elision-tokenfilter.html. Anyway, I'd suggest we discuss and make any changes in a separate bug.

Absolutely, I only talked about it to give some context. The important part for me is that the 7xx fields should be used to find an author while typing Name + Surname and that it doesn't yet. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78094|0 |1 is obsolete| | --- Comment #119 from David Gustafsson <glasklas@gmail.com> --- Created attachment 78588 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78588&action=edit Bug 19893: Restore and fix removed tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78587|0 |1 is obsolete| | --- Comment #121 from David Gustafsson <glasklas@gmail.com> --- Created attachment 78590 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78590&action=edit Bug 19893: Fix tests and add tests for joined subfields -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78588|0 |1 is obsolete| | --- Comment #123 from David Gustafsson <glasklas@gmail.com> --- Created attachment 78592 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78592&action=edit Bug 19893: Restore and fix removed tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78590|0 |1 is obsolete| | --- Comment #125 from David Gustafsson <glasklas@gmail.com> --- Created attachment 78594 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78594&action=edit Bug 19893: Fix tests and add tests for joined subfields -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #127 from Ere Maijala <ere.maijala@helsinki.fi> --- Maybe I'm missing something, but this is still not quite working as I was expecing. With "245ab" the subfields are still being split to multiple fields, and I have to use "245(ab)" to put them in a single field. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #129 from Ere Maijala <ere.maijala@helsinki.fi> --- So having two rules like this: 245a 245b Is now same as 245ab right? Since you can already list non-joined fields separately, is there any reason to actually need the parenthesis for joining? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78091|0 |1 is obsolete| | --- Comment #131 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 78690 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78690&action=edit Bug 19893 - Alternative optimized indexing for Elasticsearch Implement optimized indexing for Elasticsearch How to test: 1) Time a full elasticsearch re-index without this patch by running the rebuild_elastic_search.pl with the -d flag: `koha-shell <instance_name> -c "time rebuild_elastic_search.pl -d"`. 2) Apply this patch. 3) Time a full re-index again, it should be about twice at fast (for a couple of thousand biblios, with fewer biblios results may be more unpredictable). Sponsored-by: Gothenburg University Library Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78093|0 |1 is obsolete| | --- Comment #133 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 78692 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78692&action=edit Bug 19893: Add index status Add persistent per index "index status" state to provide useful user feedback when update of Elasticsearch server mappings fails Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78592|0 |1 is obsolete| | --- Comment #135 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 78694 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78694&action=edit Bug 19893: Restore and fix removed tests Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78594|0 |1 is obsolete| | --- Comment #137 from Ere Maijala <ere.maijala@helsinki.fi> --- Created attachment 78696 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78696&action=edit Bug 19893: Fix tests and add tests for joined subfields Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Ere Maijala <ere.maijala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #138 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Quick (naive?) questions: - Why do you loop on MARC fields instead of the mappings? - What are "joined fields", do we really to index everything? - Can you submit an example (record in input will output this document)? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #140 from Ere Maijala <ere.maijala@helsinki.fi> --- Forgot to mention that joining is also important for faceting. There are some examples of what you get in t/Koha/SearchEngine/Elasticsearch.t. It obviously depends on the rules. But here's a short example of the 245 field. Input: 245 $aMain title / $bSubtitle Rule: title = 245(ab) Result: title: Main title / Subtitle Without joining: Input: 245 $aMain title / $bSubtitle Rule: title = 245ab Result: title: Main title title: Subtitle -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #142 from David Gustafsson <glasklas@gmail.com> --- (In reply to Ere Maijala from comment #130)

Oh, I see the light now. This is useful also for indexing all subfields without having to list all of them. Looks good to me then. We should just make sure to document how the rules work.

Yes, exactly. I was going to make this point but you made it for me :) Besides being cumbersome it would also make the mappings.yaml quite huge and difficult to read, and mappings slower to fetch from database. Also, the mappings are already defined in a way that you need to make minimal changes with this approach. If joing was the default it would have to be largely rewritten. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #144 from David Gustafsson <glasklas@gmail.com> --- (In reply to Jonathan Druart from comment #138)

Quick (naive?) questions: - Why do you loop on MARC fields instead of the mappings? - What are "joined fields", do we really to index everything? - Can you submit an example (record in input will output this document)?

About the first one. This is actually one of the tricks employed to make this much faster than catmandu. If you look at the "field" subroutine, it loops through all the fields on each call. So if we where to instead loop through the fields in mapping.yaml, and call fields for each one (like catmandu does), this is quite expensive to do. If we instead group all mappings by field stored in a hash, and loop through fields just once (by calling "field" instead) we get a huge performance gain since hash key lookup is constant time. If we add more mappings, only the parsing of mappings and creation of the rules hash will take longer, but that only has to be done once per batch so will have almost no impact on performance. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #146 from Alex Arnaud <alex.arnaud@biblibre.com> --- (In reply to David Gustafsson from comment #142)

(In reply to Ere Maijala from comment #130)

...

Oh, I see the light now. This is useful also for indexing all subfields without having to list all of them. Looks good to me then. We should just make sure to document how the rules work.

Yes, exactly. I was going to make this point but you made it for me :) Besides being cumbersome it would also make the mappings.yaml quite huge and difficult to read, and mappings slower to fetch from database. Also, the mappings are already defined in a way that you need to make minimal changes with this approach. If joing was the default it would have to be largely rewritten.

Indeed, this is useful for configuring mappings faster. I agree. syntax like 245(ab) seems good and "user friendly" to me. Agree too. But i'm wondering about updates. There's currently some libraries in production with Elasticseach. Will they need to change the rules themselves or will we provide an update script that will change the rules automatically ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #148 from David Gustafsson <glasklas@gmail.com> --- If bug 19707 was to be accepted into Koha, new mappings could also be added that way, even if automatic syncing comes with it's own problems (old mappings are not purged since can't differentiate between those and those added by end user). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #150 from Alex Arnaud <alex.arnaud@biblibre.com> --- (In reply to David Gustafsson from comment #149)

I will also try to have a look whether the structure of auto-suggest "input"-fields is correct or not.

I don't know if it works or not, i don't how to test (is this used in Koha anywhere?), but the structure is still different from Catmandu results. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Alex Arnaud <alex.arnaud@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #152 from Alex Arnaud <alex.arnaud@biblibre.com> --- First QA things here: Pod is missing for this subs: get_elasticsearch marc_records_to_documents get_marc_mapping_rules bulk_index create_index index_exists update_mappings index_status index_status_ok index_status_recreate_required index_status_reindex_required You removed ElasticsearchMARCSerializationFormat syspref, previously created, but it is still used in Koha/SearchEngine/Elasticsearch.pm =================================== Questions: I'm wondering what the "index status" feature is for. index_status sub is always used with a parameter (to set it) but never without parameter. So it seems the status is never used, right? Also, status is stored in a systempreference and can only be set by the API (not by a librarian via UI). Can we actually say that this should be a systempreference? I still have concerns about update/migration. As you changed the way we write mapping, could you provide a connection between old and new mapping syntax ? This could be useful for creating migration script (thing i'll surely do for some libraries using ES in production). And it could be a good thing to provide a (at least short) documentation about how to write mapping since we use our own syntax instead of the already documented catmandu fix language. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #154 from David Gustafsson <glasklas@gmail.com> --- Ok! No problem, will have a look at and fix these issues, it could take a week or so before I have the time though. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20768 --- Comment #155 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I'm hoping to get back to QAing this soon.. have you had a chance to take a look at the feedback to date yet David? It would be great to get this moving again and try and push it before 18.11 feature slush. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20768 [Bug 20768] Noisy test results when SearchEngine set to Elasticsearch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #156 from Ere Maijala <ere.maijala@helsinki.fi> --- I agree. David, please let me know if you need a hand with the final touches. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #158 from David Gustafsson <glasklas@gmail.com> --- Created attachment 80969 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80969&action=edit Bug 19893: Add pods, remove syspref, add tests for serialization format Add missing pods, remove obsolete syspref and add test for serialization format for records exceeding max record size -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #80970|0 |1 is obsolete| | --- Comment #160 from David Gustafsson <glasklas@gmail.com> --- Created attachment 80971 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80971&action=edit Bug 19893: Add pods, remove syspref, add tests for serialization format Add missing pods, remove obsolete syspref and add test for serialization format for records exceeding max record size -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff --- Comment #162 from David Gustafsson <glasklas@gmail.com> --- Or should perhaps be "Needs Signoff" since also did some minor refactoring. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #164 from David Gustafsson <glasklas@gmail.com> --- Created attachment 80983 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80983&action=edit Bug 19893: Fixed some typos -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #166 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to David Gustafsson from comment #164)

Created attachment 80983 [details] [review] Bug 19893: Fixed some typos

Please squash this to the previous patch as a new commit created by it is not necessary (https://wiki.koha-community.org/wiki/Development_IRC_meeting_27_September_20...) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #81014|0 |1 is obsolete| | --- Comment #170 from David Gustafsson <glasklas@gmail.com> --- Created attachment 81015 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81015&action=edit Bug 19893: Add pods, remove syspref, add tests for serialization format Add missing pods, remove obsolete syspref and add test for serialization format for records exceeding max record size -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #174 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to David Gustafsson from comment #173)

I agree It might benefit from documentation, it should (or even can) not be moved outside though, since I think there are some closures within. It is also a specific and private helper-method, thus belongs inside the method.

I couldn't see any closures in the two subroutines I mentioned (and with "closure" I mean they don't refer to any variables in the outer subroutine). The search functionality and indexing seemed to work fine also when they are in the top level (I tested). Anyways, I put this topic on the Koha developer meeting list and maybe we will get a better consensus on how to do this. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #176 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Nick Clemens from comment #175)

The subroutine should be moved to the top level

Or moved to Koha::FieldMappings. Tests must be provided in either case. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #177 from David Gustafsson <glasklas@gmail.com> --- I can have a look at this an move the subs out, I confused them with another case where closures where used, so no closures thus they work also outside. I don't really agree this is an improvement since the subs are not supposed to be used outside of the subroutines in which they are defined, but it's no big deal. Don't think Koha::FieldMappings is the right way to put them though sinceis an internal data structure used by _process_mappings, that has no purpose outside of that subroutine. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #179 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- (In reply to Jonathan Druart from comment #176)

Or moved to Koha::FieldMappings. Tests must be provided in either case.

Koha::FieldMappings is not really related to the MARC to Elasticsearch mappings that we are talking here, since Koha::FieldMappings doesn't have rules/mappings whether the field or combination of fields is a boolean, sortable, and so on. The Elasticsearch.pm module is already object oriented and the functions we are now talking about are just helper functions that don't necessarily need to be object oriented. So I'm all in for the current solution David is suggesting (but little refactoring is needed). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #181 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Hi, could you still split the changes to their own commits? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #183 from David Gustafsson <glasklas@gmail.com> --- What you could do is to to click "Show Obsolute" and download the two latest patches, then run "interdiff old_patch.patch new_patch.patch" and you will get the latest changes. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #184 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- If one big commit is fine for everybody else then I can pass it through this time. And if you come across the situation again where you have to change the commit or split them up you can do git reset --soft HEAD~1 and then just write new commit messages and in the end check with git diff whether the code is still identical to the situation before you started. I will do the actual code review hopefully by the end of this week. -- You are receiving this mail because: You are watching all bug changes.