Bug 10462 - Some optimizations in Z3950 search paving the way for enhancements
Summary: Some optimizations in Z3950 search paving the way for enhancements
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Z39.50 / SRU / OpenSearch Servers (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 22532
  Show dependency treegraph
 
Reported: 2013-06-13 13:14 UTC by Marcel de Rooy
Modified: 2019-03-18 15:57 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10462: Some optimizations in Z3950 search paving the way for enhancements (10.25 KB, patch)
2013-06-13 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10462: Some optimizations in Z3950 search paving the way for enhancements (10.27 KB, patch)
2013-06-14 11:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10462: Followup for showing multiple ISBNs in Z3950 response (2.66 KB, patch)
2013-07-03 12:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10462: Some optimizations in Z3950 search paving the way for enhancements (10.33 KB, patch)
2013-07-03 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10462: Followup for showing multiple ISBNs in Z3950 response (2.68 KB, patch)
2013-07-03 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10462: QA Followup for LCCN in Breeding.pm (1.26 KB, patch)
2013-07-24 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10462: QA Followup to resolve LCCN mixup and remove hardcoded marc tags (3.85 KB, patch)
2013-07-24 14:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-06-13 13:14:37 UTC
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.
Comment 1 Marcel de Rooy 2013-06-13 13:23:46 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-06-14 11:04:35 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-06-27 13:02:53 UTC
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.
Comment 4 Marcel de Rooy 2013-06-27 13:08:55 UTC
(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.
Comment 5 Marcel de Rooy 2013-07-03 12:00:05 UTC
Looking at this now.
Comment 6 Marcel de Rooy 2013-07-03 12:54:27 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2013-07-03 12:58:39 UTC
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 :)
Comment 8 Jonathan Druart 2013-07-03 14:34:07 UTC
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.
Comment 9 Jonathan Druart 2013-07-03 14:34:26 UTC
Created attachment 19374 [details] [review]
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>
Comment 10 Jonathan Druart 2013-07-03 14:34:32 UTC
Created attachment 19375 [details] [review]
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>
Comment 11 Galen Charlton 2013-07-23 15:42:14 UTC
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?
Comment 12 Marcel de Rooy 2013-07-24 11:27:30 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2013-07-24 11:53:19 UTC
> 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 :)
Comment 14 Galen Charlton 2013-07-24 14:38:39 UTC
(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',
}
Comment 15 Marcel de Rooy 2013-07-24 14:52:11 UTC
Created attachment 19928 [details] [review]
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).
Comment 16 Marcel de Rooy 2013-07-24 14:55:40 UTC
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..
Comment 17 Galen Charlton 2013-07-24 16:57:49 UTC
Pushed to master, along with a follow-up to get rid of some undefined variable warnings.  Thanks, Marcel!