Bugzilla – Attachment 81822 Details for
Bug 19893
Alternative optimized indexing for Elasticsearch
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 19893: Add code review fixes
Bug-19893-Add-code-review-fixes.patch (text/plain), 8.11 KB, created by
David Gustafsson
on 2018-11-01 14:47:47 UTC
(
hide
)
Description:
Bug 19893: Add code review fixes
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2018-11-01 14:47:47 UTC
Size:
8.11 KB
patch
obsolete
>From 1c7799dbdbe4d16681a942e1edd5a6be0ccd6e84 Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >Date: Thu, 1 Nov 2018 15:47:12 +0100 >Subject: [PATCH] Bug 19893: Add code review fixes > >--- > Koha/SearchEngine/Elasticsearch.pm | 61 ++++++++++++++++------------ > admin/searchengine/elasticsearch/mappings.pl | 2 +- > t/Koha/SearchEngine/Elasticsearch.t | 2 +- > 3 files changed, 36 insertions(+), 29 deletions(-) > >diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm >index 1c5fc29d4b..d35f61f1d5 100644 >--- a/Koha/SearchEngine/Elasticsearch.pm >+++ b/Koha/SearchEngine/Elasticsearch.pm >@@ -35,7 +35,6 @@ use Try::Tiny; > use YAML::Syck; > > use List::Util qw( sum0 reduce ); >-use Search::Elasticsearch; > use MARC::File::XML; > use MIME::Base64; > use Encode qw(encode); >@@ -312,17 +311,22 @@ sub sort_fields { > > =head2 _process_mappings($mappings, $data, $record_document) > >-Process all C<$mappings> targets operating on a specific MARC field C<$data> applied to C<$record_document> >-Since we group all mappings by MARC field targets C<$mappings> will contain all targets for C<$data> >-and thus we need to fetch the MARC field only once. >+ $self->_process_mappings($mappings, $marc_field_data, $record_document) >+ >+Process all C<$mappings> targets operating on a specific MARC field C<$data>. >+Since we group all mappings by MARC field targets C<$mappings> will contain >+all targets for C<$data> and thus we need to fetch the MARC field only once. >+C<$mappings> will be applied to C<$record_document> and new field values added. >+The method has no return value. > > =over 4 > > =item C<$mappings> > >-Arrayref of mappings containing arrayrefs on the format [C<$taget>, C<$options>] where >-C<$target> is the name of the target field and C<$options> is a hashref containing processing >-directives for this particular mapping. >+Arrayref of mappings containing arrayrefs on the format >+[C<$taget>, C<$options>] where C<$target> is the name of the target field and >+C<$options> is a hashref containing processing directives for this particular >+mapping. > > =item C<$data> > >@@ -330,7 +334,8 @@ The source data from a MARC record field. > > =item C<$record_document> > >-Hashref representing the Elasticsearch document on which mappings should be applied. >+Hashref representing the Elasticsearch document on which mappings should be >+applied. > > =back > >@@ -482,15 +487,20 @@ sub marc_records_to_documents { > > =head2 _field_mappings($facet, $suggestible, $sort, $target_name, $target_type, $range) > >-Get mappings, an internal data structure later used by L<_process_mappings($mappings, $data, $record_document)> >-to process MARC target data, for a MARC mapping. >+ my @mappings = _field_mappings($facet, $suggestible, $sort, $target_name, $target_type, $range) >+ >+Get mappings, an internal data structure later used by >+L<_process_mappings($mappings, $data, $record_document)> to process MARC target >+data for a MARC mapping. > >-The returned C<$mappings> is to to be confused with mappings provided by C<_foreach_mapping>, rather this >-sub accepts properties from a mapping as provided by C<_foreach_mapping> and expands it to this internal >-data stucture. In the caller context (C<_get_marc_mapping_rules>) the returned C<@mappings> is then >-applied to each MARC target (leader, control field data, subfield or joined subfields) and >-integrated into the mapping rules data structure used in C<marc_records_to_documents> to >-transform MARC records into Elasticsearch documents. >+The returned C<$mappings> is not to to be confused with mappings provided by >+C<_foreach_mapping>, rather this sub accepts properties from a mapping as >+provided by C<_foreach_mapping> and expands it to this internal data stucture. >+In the caller context (C<_get_marc_mapping_rules>) the returned C<@mappings> >+is then applied to each MARC target (leader, control field data, subfield or >+joined subfields) and integrated into the mapping rules data structure used in >+C<marc_records_to_documents> to transform MARC records into Elasticsearch >+documents. > > =over 4 > >@@ -516,14 +526,14 @@ Elasticsearch document target field type. > > =item C<$range> > >-An optinal range as a string on the format "<START>-<END>" or "<START>", >+An optional range as a string on the format "<START>-<END>" or "<START>", > where "<START>" and "<END>" are integers specifying a range that will be used >-for extracting a substing from MARC data as Elasticsearch field target value. >+for extracting a substring from MARC data as Elasticsearch field target value. > > The first character position is "1", and the range is inclusive, > so "1-3" means the first three characters of MARC data. > >-If only "<START>" is provided only one character as position "<START>" will >+If only "<START>" is provided only one character at position "<START>" will > be extracted. > > =back >@@ -593,7 +603,7 @@ rules keyed by MARC field tags holding all the mapping rules for that particular > > We can then iterate through all MARC fields for each record and apply all relevant > rules once per fields instead of retreiving fields multiple times for each mapping rule >-wich is terribly slow. >+which is terribly slow. > > =cut > >@@ -604,10 +614,7 @@ wich is terribly slow. > > sub _get_marc_mapping_rules { > my ($self) = @_; >- > my $marcflavour = lc C4::Context->preference('marcflavour'); >- my @rules; >- > my $field_spec_regexp = qr/^([0-9]{3})([()0-9a-z]+)?(?:_\/(\d+(?:-\d+)?))?$/; > my $leader_regexp = qr/^leader(?:_\/(\d+(?:-\d+)?))?$/; > my $rules = { >@@ -625,7 +632,7 @@ sub _get_marc_mapping_rules { > if ($type eq 'sum') { > push @{$rules->{sum}}, $name; > } >- elsif($type eq 'boolean') { >+ elsif ($type eq 'boolean') { > # boolean gets special handling, if value doesn't exist for a field, > # it is set to false > $rules->{defaults}->{$name} = 'false'; >@@ -644,7 +651,7 @@ sub _get_marc_mapping_rules { > foreach my $token (split //, $2) { > if ($token eq "(") { > if ($open_group) { >- die("Unmatched opening parenthesis for $marc_field"); >+ croak "Unmatched opening parenthesis for $marc_field"; > } > else { > $open_group = 1; >@@ -659,7 +666,7 @@ sub _get_marc_mapping_rules { > $open_group = 0; > } > else { >- die("Unmatched closing parenthesis for $marc_field"); >+ croak "Unmatched closing parenthesis for $marc_field"; > } > } > elsif ($open_group) { >@@ -699,7 +706,7 @@ sub _get_marc_mapping_rules { > push @{$rules->{leader}}, @mappings; > } > else { >- die("Invalid MARC field: $marc_field"); >+ croak "Invalid MARC field: $marc_field"; > } > }); > return $rules; >diff --git a/admin/searchengine/elasticsearch/mappings.pl b/admin/searchengine/elasticsearch/mappings.pl >index ab0b6538f4..49a11918ce 100755 >--- a/admin/searchengine/elasticsearch/mappings.pl >+++ b/admin/searchengine/elasticsearch/mappings.pl >@@ -47,7 +47,7 @@ my $schema = $database->schema; > > my $marc_type = lc C4::Context->preference('marcflavour'); > >-my @index_names = ('biblios', 'authorities'); >+my @index_names = ($Koha::SearchEngine::BIBLIOS_INDEX, $Koha::SearchEngine::AUTHORITIES_INDEX); > > my $update_mappings = sub { > for my $index_name (@index_names) { >diff --git a/t/Koha/SearchEngine/Elasticsearch.t b/t/Koha/SearchEngine/Elasticsearch.t >index 323a3b0106..89493f23b9 100644 >--- a/t/Koha/SearchEngine/Elasticsearch.t >+++ b/t/Koha/SearchEngine/Elasticsearch.t >@@ -220,7 +220,7 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests' > } > }); > >- my $see = Koha::SearchEngine::Elasticsearch->new({ index => 'biblios' }); >+ my $see = Koha::SearchEngine::Elasticsearch->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); > > my $marc_record_1 = MARC::Record->new(); > $marc_record_1->leader(' cam 22 a 4500'); >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 19893
:
70199
|
70200
|
70201
|
70202
|
70230
|
70325
|
73153
|
75109
|
75271
|
75272
|
75353
|
75428
|
75455
|
75588
|
75658
|
75660
|
76612
|
76613
|
76614
|
76629
|
78091
|
78092
|
78093
|
78094
|
78524
|
78561
|
78587
|
78588
|
78589
|
78590
|
78591
|
78592
|
78593
|
78594
|
78690
|
78691
|
78692
|
78693
|
78694
|
78695
|
78696
|
80969
|
80970
|
80971
|
80983
|
81014
|
81015
|
81284
|
81822
|
81901
|
81903
|
81955
|
81957
|
81958
|
81959
|
81960
|
81961
|
81962
|
81963
|
81964
|
81965
|
81966
|
82143
|
82144
|
82145
|
82146
|
82147
|
82148
|
82149
|
82150
|
82151
|
82152
|
82256
|
82257
|
82258
|
82259
|
82260
|
82261
|
82262
|
82263
|
82264
|
82265
|
82266
|
82267