https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |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=24715 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99514|0 |1 is obsolete| | --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 99515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99515&action=edit Bug 24715: Cache repeatable subfield in TransformKohaToMarc Implemented by calling GetMarcSubfieldStructure. Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Optionally, follow the test plan of bug 21800#comment7. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #99515|0 |1 is obsolete| | --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 99516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=99516&action=edit Bug 24715: Cache repeatable subfield in TransformKohaToMarc Implemented by calling GetMarcSubfieldStructure. Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Optionally, follow the test plan of bug 21800#comment7. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Ere Maijala from comment #5)

I think that for this to help the worst case, e.g. export of large number of records with items that have repeated subfields, the framework data should be cached using Koha::Cache like e.g. C4::Biblio::GetMarcStructure does.

+ $mss2 = GetMarcSubfieldStructure($params->{framework}, { unsafe => 1 }) You mean this ? This comes from the cache. -- You are receiving this mail because: You are watching all bug changes.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Ere Maijala from comment #8)

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>

Great! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #10)

1994 my $mss2; # belongs to $params->{framework} only filled when needed

I don't understand, can you explain? $mss2 in TransformKohaToMarc is never assigned.

Ahh you might be right. I am passing undef to the routine that assigns it. So that wont make sense. If I would pass a reference, it should be fine I guess? See sub _check_split. Not the most transparent code indeed. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Sorry but I do not understand exactly what you are trying to do, so it's hard to help :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 102993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102993&action=edit Bug 24715: Improve code of _check_split This routine may check repeatability in a specific framework, and return the framework hash. The handling of $mss2 to store that hash is improved here. Adding a test in TransformKohaToMarc.t to see if we hit the cache. Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I think I understand what you are trying to do but, did you benchmark it? You a putting in a variable something that is L1 cached, then retrieved from there without deep copy. I am really not sure it is needed. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Marcel de Rooy from comment #17)

(In reply to Jonathan Druart from comment #16)

...

I think I understand what you are trying to do but, did you benchmark it?

You a putting in a variable something that is L1 cached, then retrieved from there without deep copy.

I am really not sure it is needed.

Yeah, using the unsafe parameter as done elsewhere. About cache/benchmarking, see bug 21800 comment 12. I added it on request. Note that I only check the framework if it is really needed. So when we actually get a repeated value in a field.

I am not against the unsafe parameter. My concern is more about the usefulness of the $mss2 trick. I have the conviction that is not needed, retrieving $mss (framework specific in _check_split) from the L1 cache should be the only thing you need here. Basically the patch could only be: # here we need to check the specific framework my $mss = GetMarcSubfieldStructure($params->{framework}, { unsafe => 1 }); foreach my $fld2 ( @{ $mss->{ $fld->{kohafield} } } ) { next if $fld2->{tagfield} ne $fld->{tagfield}; next if $fld2->{tagsubfield} ne $fld->{tagsubfield}; return 1 if $fld2->{repeatable}; } -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103005|0 |1 is obsolete| | --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 103006 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103006&action=edit Bug 24715: (QA follow-up) Remove $mss2 tric As requested by QA, removing the $mss2 variable. This may lead to repeated calls to the cache. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102992|0 |1 is obsolete| | Attachment #102993|0 |1 is obsolete| | Attachment #103006|0 |1 is obsolete| | --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103010 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103010&action=edit Bug 24715: Cache repeatable subfield in TransformKohaToMarc Implemented by calling GetMarcSubfieldStructure. Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Optionally, follow the test plan of bug 21800#comment7. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi> Bug 24715: Improve code of _check_split This routine may check repeatability in a specific framework, and return the framework hash. The handling of $mss2 to store that hash is improved here. Adding a test in TransformKohaToMarc.t to see if we hit the cache. Test plan: Run t/db_dependent/Biblio/TransformKohaToMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 24715: (QA follow-up) Remove $mss2 tric As requested by QA, removing the $mss2 variable. This may lead to repeated calls to the cache. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Patches have been squashed for readability. I also re-add the existing syntax for the _check_split call -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #22)

This patch could be slower in some cases, but will bring few DB hits in the majority of the cases I think. Especially if we assume that the framework is already cached in L1.

s/bring/save -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24715 --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.