Bugzilla – Attachment 81901 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), 10.53 KB, created by
David Gustafsson
on 2018-11-02 14:55:18 UTC
(
hide
)
Description:
Bug 19893: Add code review fixes
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2018-11-02 14:55:18 UTC
Size:
10.53 KB
patch
obsolete
>From 9e7382aba35ae204509bb4643a2451201f0ae2b7 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/Exceptions/Elasticsearch.pm | 17 +++++++ > Koha/SearchEngine/Elasticsearch.pm | 71 ++++++++++++++++------------ > Koha/SearchEngine/Elasticsearch/Indexer.pm | 2 +- > admin/searchengine/elasticsearch/mappings.pl | 2 +- > t/Koha/SearchEngine/Elasticsearch.t | 2 +- > 5 files changed, 62 insertions(+), 32 deletions(-) > create mode 100644 Koha/Exceptions/Elasticsearch.pm > >diff --git a/Koha/Exceptions/Elasticsearch.pm b/Koha/Exceptions/Elasticsearch.pm >new file mode 100644 >index 0000000000..d4fd589fbf >--- /dev/null >+++ b/Koha/Exceptions/Elasticsearch.pm >@@ -0,0 +1,17 @@ >+package Koha::Exceptions::Elasticsearch; >+ >+use Modern::Perl; >+ >+use Exception::Class ( >+ >+ 'Koha::Exceptions::Elasticsearch' => { >+ description => 'Something went wrong!', >+ }, >+ 'Koha::Exceptions::Elasticsearch::MARCFieldExprParseError' => { >+ isa => 'Koha::Exceptions::Elasticsearch', >+ description => 'Parse error while processing MARC field expression in mapping', >+ } >+ >+); >+ >+1; >diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm >index 1c5fc29d4b..bb9a1140a3 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); >@@ -69,7 +68,7 @@ sub new { > my $class = shift @_; > my $self = $class->SUPER::new(@_); > # Check for a valid index >- croak('No index name provided') unless $self->index; >+ Koha::Exceptions::MissingParameter->throw('No index name provided') unless $self->index; > return $self; > } > >@@ -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 in 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 > >@@ -465,7 +470,7 @@ sub marc_records_to_documents { > # Suppress warnings if record length exceeded > unless (substr($record->leader(), 0, 5) eq '99999') { > foreach my $warning (@warnings) { >- carp($warning); >+ carp $warning; > } > } > $record_document->{'marc_data'} = $record->as_xml_record($marcflavour); >@@ -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) > >-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. >+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 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 in 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,9 @@ sub _get_marc_mapping_rules { > foreach my $token (split //, $2) { > if ($token eq "(") { > if ($open_group) { >- die("Unmatched opening parenthesis for $marc_field"); >+ Koha::Exceptions::Elasticsearch::MARCFieldExprParseError->throw( >+ "Unmatched opening parenthesis for $marc_field" >+ ); > } > else { > $open_group = 1; >@@ -659,7 +668,9 @@ sub _get_marc_mapping_rules { > $open_group = 0; > } > else { >- die("Unmatched closing parenthesis for $marc_field"); >+ Koha::Exceptions::Elasticsearch::MARCFieldExprParseError->throw( >+ "Unmatched closing parenthesis for $marc_field" >+ ); > } > } > elsif ($open_group) { >@@ -699,7 +710,9 @@ sub _get_marc_mapping_rules { > push @{$rules->{leader}}, @mappings; > } > else { >- die("Invalid MARC field: $marc_field"); >+ Koha::Exceptions::Elasticsearch::MARCFieldExprParseError->throw( >+ "Invalid MARC field expression: $marc_field" >+ ); > } > }); > return $rules; >diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm >index 0bc68f6435..4774d78019 100644 >--- a/Koha/SearchEngine/Elasticsearch/Indexer.pm >+++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm >@@ -78,7 +78,7 @@ use constant { > try { > $self->update_index($biblionums, $records); > } catch { >- die("Something whent wrong trying to update index:" . $_[0]); >+ die("Something went wrong trying to update index:" . $_[0]); > } > > Converts C<MARC::Records> C<$records> to Elasticsearch documents and performs >diff --git a/admin/searchengine/elasticsearch/mappings.pl b/admin/searchengine/elasticsearch/mappings.pl >index ab0b6538f4..28faa73659 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::Elasticsearch::BIBLIOS_INDEX, $Koha::SearchEngine::Elasticsearch::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..993cb1bf8e 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::Elasticsearch::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