[Bug 10462] New: Some optimizations in Z3950 search paving the way for enhancements
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Bug ID: 10462 Summary: Some optimizations in Z3950 search paving the way for enhancements Classification: Unclassified Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Z39.50 / SRU / OpenSearch Servers Assignee: m.de.rooy@rijksmuseum.nl Reporter: m.de.rooy@rijksmuseum.nl CC: m.de.rooy@rijksmuseum.nl In order to achieve the goals of the older bug 6536, this proposed patch refactors some code in Z3950 search. Behavior does not change; should be somewhat faster though. This patch also disables batch record counts for Z3950 search records; these counts are not used anywhere in Koha. It prepares removal of the z3950random column, which also is of no use. This report should be followed by SRU targets, and XSLT processing. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #1 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 18939 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18939&action=edit Bug 10462: Some optimizations in Z3950 search paving the way for enhancements Refactors Z3950Search. Disable batch record counts for z3950 records. Test plan: Do various Z3950 searches on multiple targets from Cataloging and Acquisition. Behavior should not have changed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=6536 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18939|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 18983 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18983&action=edit Bug 10462: Some optimizations in Z3950 search paving the way for enhancements Refactors Z3950Search. Disable batch record counts for z3950 records. Test plan: Do various Z3950 searches on multiple targets from Cataloging and Acquisition. Behavior should not have changed. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #3 from Jonathan Druart <jonathan.druart@biblibre.com> --- Hi Marcel, On searching isbn=2070518426 on the BNF Z3950 server, I got on master: Harry Potter à l'école des sorciers # J. K. Rowling # # 2070518426 | 9782070612369 and, on master + your patch: Harry Potter à l'école des sorciers # J. K. Rowling # 1998 # 2070518426 I separate fields with '#'. With your patch, the date is get back but the isbn field seems truncated: on master I got 2 isbn separated with a pipe, now I get only one isbn. Looking at the code, I cannot explain this change. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #4 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #3)
Hi Marcel,
On searching isbn=2070518426 on the BNF Z3950 server, I got on master: Harry Potter à l'école des sorciers # J. K. Rowling # # 2070518426 | 9782070612369 and, on master + your patch: Harry Potter à l'école des sorciers # J. K. Rowling # 1998 # 2070518426
I separate fields with '#'.
With your patch, the date is get back but the isbn field seems truncated: on master I got 2 isbn separated with a pipe, now I get only one isbn.
Looking at the code, I cannot explain this change.
Will soon have a look at this. Thanks. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #5 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking at this now. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #6 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 19366 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19366&action=edit Bug 10462: Followup for showing multiple ISBNs in Z3950 response As Jonathan correctly noted, the new Z3950 response only showed one isbn although more isbn numbers could be in the record and would be imported. To resolve this display problem, I traverse them all now in the updated routine _isbn_show. There is no change in the imported records. Note that before this patch TransformMarcToKoha did put all isbn numbers in one field, separated by pipes (for display only). This behavior is restored now. The three regexes on the individual isbn numbers now seem to be overkill, but I left them there for completeness. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested this on a fresh French install under UNIMARC with BNF server. Tested it too for MARC21. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #7 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Jonathan, Thanks for testing. Good catch for the isbn numbers. Note that it only was a display problem. As explained on the followup commit message, this has been resolved now. You are more than welcome to check again :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Medium patch QA Contact| |jonathan.druart@biblibre.co | |m --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- QA comment: Looking at the code, nothing to say. It is not perfect, but it has the merit of being better than before (using refactoring). I did not find any regression with the last patch. Marked as Passed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18983|0 |1 is obsolete| | --- Comment #9 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 19374 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19374&action=edit Bug 10462: Some optimizations in Z3950 search paving the way for enhancements Refactors Z3950Search. Disable batch record counts for z3950 records. Test plan: Do various Z3950 searches on multiple targets from Cataloging and Acquisition. Behavior should not have changed. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19366|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 19375 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19375&action=edit Bug 10462: Followup for showing multiple ISBNs in Z3950 response As Jonathan correctly noted, the new Z3950 response only showed one isbn although more isbn numbers could be in the record and would be imported. To resolve this display problem, I traverse them all now in the updated routine _isbn_show. There is no change in the imported records. Note that before this patch TransformMarcToKoha did put all isbn numbers in one field, separated by pipes (for display only). This behavior is restored now. The three regexes on the individual isbn numbers now seem to be overkill, but I left them there for completeness. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested this on a fresh French install under UNIMARC with BNF server. Tested it too for MARC21. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA CC| |gmcharlt@gmail.com --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> --- This patch changes what gets returned as the LCCN. The acronym "LCCN" is ambiguous, unfortunately, as it can refer to LC Control Number or LC Call Number. The current behavior of the Z39.50 search is to return the former (i.e., the control number or LC-assigned identification number). Here is a diff (before and after applying the patch) of the output of a little test script I wrote that searches LC's Z39.50 server for ISBN 9780415964845. 'total_pages' => 1, 'breeding_loop' => [ { - 'lccn' => ' 2008005439', - 'breedingid' => '91', - 'date' => '2008', - 'author' => undef, + 'lccn' => 'PR6068.O93', + 'date' => '2008.', + 'breedingid' => '92', + 'author' => '', 'biblionumber' => undef, 'edition' => '2nd ed.', 'server' => 'LIBRARY OF CONGRESS', I'm marking this as failed QA for the change in visible behavior, but this issue speaks to a broader concern I have with this patch: why replace the call to TransformMarcToKoha, which uses the installed default bibliographic framework to extract strings from the MARC record, with hardcoding the tags and subfields for each MARC flavor? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #12 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 19926 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19926&action=edit Bug 10462: QA Followup for LCCN in Breeding.pm Subroutine for Z3950 search in C4/Breeding.pmxxx interpreted LCCN as LC call number (tag 050), but should be control number (tag 010). Test plan: Check LCCN output on z3950_search.pl results table with and without patch. Search e.g. on ISBN 9780415964845. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #13 from M. de Rooy <m.de.rooy@rijksmuseum.nl> ---
This patch changes what gets returned as the LCCN. The acronym "LCCN" is ambiguous, unfortunately, as it can refer to LC Control Number or LC Call Number. The current behavior of the Z39.50 search is to return the former (i.e., the control number or LC-assigned identification number).
Good catch. Apparently mixed up these two tags with same acronym. The small followup corrects that.
Why replace the call to TransformMarcToKoha, which uses the installed default bibliographic framework to extract strings from the MARC record, with hardcoding the tags and subfields for each MARC flavor?
Good question. The main reason was just optimization. I saw that we needed only six fields from the MARC record for a simple display, and TransformMarcToKoha is doing a transformation of the whole MARC record. The original code contains a call to TransformMarcToKoha followed by a call to ImportBreeding that again calls TransformMarcToKoha. Both calls have been optimized. 'Downside' is indeed a few lines hardcoding tags in the sub _add_rowdata. Let me still see a moment if we can still improve some there in order to gain more confidence :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #14 from Galen Charlton <gmcharlt@gmail.com> --- (In reply to M. de Rooy from comment #13)
Good question. The main reason was just optimization. I saw that we needed only six fields from the MARC record for a simple display, and TransformMarcToKoha is doing a transformation of the whole MARC record. The original code contains a call to TransformMarcToKoha followed by a call to ImportBreeding that again calls TransformMarcToKoha. Both calls have been optimized. 'Downside' is indeed a few lines hardcoding tags in the sub _add_rowdata. Let me still see a moment if we can still improve some there in order to gain more confidence :)
I suspected it would be something like that. Arguably that's at the level of micro-optimization that could be dispensed with in the interests of keeping the code general, but a possible middle ground that might speed things up would be writing a routine like this: $hashref = ExtractFieldsPerFrameWork($marcrecord, $arrayref_of_fields, $frameworkcode); e.g. ExtractFieldsPerFrameWork($marcrecord, ['biblio.title', 'bibliitems.issn' ], ''); returning { 'biblio.title' => 'Journal of irreproducible results', 'biblioitems.issn' => '0022-2038', } -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19926|0 |1 is obsolete| | --- Comment #15 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 19928 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19928&action=edit Bug 10462: QA Followup to resolve LCCN mixup and remove hardcoded marc tags This patch corrects the mixup for LC call number and control number. Further, as suggested by Galen, it would be better to not introduce hardcoded tags in the Z3950Search subs in Breeding.pm. This patch resolves that by calling TransformMarcToKohaOneField. Note that this only involves changes to _addrowdata and _isbn_show. These subs are only used in building the displayed results table. Additionally, for French UNIMARC installs publicationyear is used to fill the Date column (copyrightdate is not used in those installs). The edition statement is only used in unimarc_lecture_pub not in unimarc_complet. Test plan: Do some Z3950 searches and look for values in all result columns. For MARC21 on LOC (and/or others): Look for isbn 9780415964845 (check LCCN). Look for author Rowling. For UNIMARC on BNF2 (and/or others): On BNF2 look for isbn 2070518426: result contains date and multiple isbn's. Look for title: Guide des candidats aux emplois de commissaire de police. Third result show edition statement (if you use 205$a with pub install). Note that there are no results with LCCN here (just as before). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested for MARC21 and UNIMARC (French lecture_pub install). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 --- Comment #16 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Galen, Saw your comment just after uploading the latest followup that hopefully covers your suggestions as well. Resetting the status. If you need additional signoff, please let me know. In that case Kyle or Jonathan may be willing to repeat it.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #17 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master, along with a follow-up to get rid of some undefined variable warnings. Thanks, Marcel! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10462 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org