@@ -, +, @@ --- C4/Biblio.pm | 7 ++- C4/Heading.pm | 38 ++++++++++-- C4/Heading/MARC21.pm | 15 +++++ C4/Matcher.pm | 12 +++- Koha/SearchEngine/Elasticsearch/Indexer.pm | 16 ++--- Koha/SearchEngine/Elasticsearch/QueryBuilder.pm | 45 ++++++++------- Koha/SearchEngine/Elasticsearch/Search.pm | 77 ++++++++++++++----------- Koha/SearchEngine/Zebra/Search.pm | 4 +- misc/link_bibs_to_authorities.pl | 15 +++-- 9 files changed, 149 insertions(+), 80 deletions(-) --- a/C4/Biblio.pm +++ a/C4/Biblio.pm @@ -554,7 +554,10 @@ sub LinkBibHeadingsToAuthorities { '', '', "a" => "" . $field->subfield('a') ); map { $authfield->add_subfields( $_->[0] => $_->[1] ) - if ( $_->[0] =~ /[A-z]/ && $_->[0] ne "a" ) + if ( $_->[0] =~ /[A-z]/ && $_->[0] ne "a" + && C4::Heading::valid_bib_heading_subfield( + $authority_type->auth_tag_to_report, $_->[0] ) + ); } $field->subfields(); $marcrecordauth->insert_fields_ordered($authfield); @@ -2970,7 +2973,7 @@ sub _koha_modify_biblio { $sth->execute( $frameworkcode, $biblio->{'author'}, $biblio->{'title'}, $biblio->{'unititle'}, $biblio->{'notes'}, - $biblio->{'serial'}, $biblio->{'seriestitle'}, $biblio->{'copyrightdate'}, $biblio->{'abstract'}, $biblio->{'biblionumber'} + $biblio->{'serial'}, $biblio->{'seriestitle'}, $biblio->{'copyrightdate'} ? int($biblio->{'copyrightdate'}) : undef, $biblio->{'abstract'}, $biblio->{'biblionumber'} ) if $biblio->{'biblionumber'}; if ( $dbh->errstr || !$biblio->{'biblionumber'} ) { --- a/C4/Heading.pm +++ a/C4/Heading.pm @@ -17,6 +17,8 @@ package C4::Heading; # You should have received a copy of the GNU General Public License # along with Koha; if not, see . +use Modern::Perl; + use strict; use warnings; use MARC::Record; @@ -170,6 +172,22 @@ sub preferred_authorities { return $results; } +=head2 valid_bib_heading_subfield + + if (C4::Heading::valid_bib_heading_subfield('100', 'e', '')) ... + +=cut + +sub valid_bib_heading_subfield { + my $tag = shift; + my $subfield = shift; + my $marcflavour = @_ ? shift : C4::Context->preference('marcflavour'); + + my $marc_handler = _marc_format_handler($marcflavour); + + return $marc_handler->valid_bib_heading_subfield( $tag, $subfield ); +} + =head1 INTERNAL METHODS =head2 _search @@ -200,12 +218,24 @@ sub _search { # push @operator, 'is'; # push @value, $self->{'thesaurus'}; # } - require C4::AuthoritiesMarc; - return C4::AuthoritiesMarc::SearchAuthorities( + + require Koha::SearchEngine::QueryBuilder; + require Koha::SearchEngine::Search; + ++ # Use state variables to avoid recreating the objects every time. ++ # With Elasticsearch this also avoids creating a massive amount of ++ # ES connectors that would eventually run out of file descriptors. + state $builder = Koha::SearchEngine::QueryBuilder->new( + { index => $Koha::SearchEngine::AUTHORITIES_INDEX } ); + state $searcher = Koha::SearchEngine::Search->new( + {index => $Koha::SearchEngine::AUTHORITIES_INDEX} ); + + my $search_query = $builder->build_authorities_query_compat( \@marclist, \@and_or, \@excluding, \@operator, - \@value, 0, 20, $self->{'auth_type'}, - 'AuthidAsc', $skipmetadata + \@value, $self->{'auth_type'}, + 'AuthidAsc' ); + return $searcher->search_auth_compat( $search_query, 0, 20, $skipmetadata ); } =head1 INTERNAL FUNCTIONS --- a/C4/Heading/MARC21.pm +++ a/C4/Heading/MARC21.pm @@ -161,6 +161,21 @@ sub valid_bib_heading_tag { } +=head2 valid_bib_heading_subfield + +=cut + +sub valid_bib_heading_subfield { + my $self = shift; + my $tag = shift; + my $subfield = shift; + + if ( exists $bib_heading_fields->{$tag} ) { + return 1 if ($bib_heading_fields->{$tag}->{subfields} =~ /$subfield/); + } + return 0; +} + =head2 parse_heading =cut --- a/C4/Matcher.pm +++ a/C4/Matcher.pm @@ -664,7 +664,10 @@ sub get_matches { #NOTE: double-quote the values so you don't get a "Embedded truncation not supported" error when a term has a ? in it. } - my $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::BIBLIOS_INDEX}); + # Use state variables to avoid recreating the objects every time. + # With Elasticsearch this also avoids creating a massive amount of + # ES connectors that would eventually run out of file descriptors. + state $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::BIBLIOS_INDEX}); ( $error, $searchresults, $total_hits ) = $searcher->simple_search_compat( $query, 0, $max_matches, undef, skip_normalize => 1 ); @@ -703,8 +706,11 @@ sub get_matches { push @operator, 'exact'; push @value, $key; } - my $builder = Koha::SearchEngine::QueryBuilder->new({index => $Koha::SearchEngine::AUTHORITIES_INDEX}); - my $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::AUTHORITIES_INDEX}); + # Use state variables to avoid recreating the objects every time. + # With Elasticsearch this also avoids creating a massive amount of + # ES connectors that would eventually run out of file descriptors. + state $builder = Koha::SearchEngine::QueryBuilder->new({index => $Koha::SearchEngine::AUTHORITIES_INDEX}); + state $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::AUTHORITIES_INDEX}); my $search_query = $builder->build_authorities_query_compat( \@marclist, \@and_or, \@excluding, \@operator, \@value, undef, 'AuthidAsc' --- a/Koha/SearchEngine/Elasticsearch/Indexer.pm +++ a/Koha/SearchEngine/Elasticsearch/Indexer.pm @@ -69,20 +69,18 @@ use constant { }; sub update_index { - my ($self, $biblionums, $records) = @_; + my ($self, $biblionums, $records, $commit) = @_; - # TODO should have a separate path for dealing with a large number - # of records at once where we use the bulk update functions in ES. if ($biblionums) { $self->_sanitise_records($biblionums, $records); } - $self->bulk_index($records); + $self->bulk_index($records, $commit); return 1; } sub bulk_index { - my ($self, $records) = @_; + my ($self, $records, $commit) = @_; my $conf = $self->get_elasticsearch_params(); my $elasticsearch = $self->get_elasticsearch(); my $documents = $self->marc_records_to_documents($records); @@ -103,6 +101,7 @@ sub bulk_index { type => 'data', # is just hard coded in Indexer.pm? body => \@body ); + $elasticsearch->flush() if ($commit); } # TODO: handle response return 1; @@ -190,8 +189,8 @@ it to be updated by a regular index cron job in the future. =cut sub update_index_background { - my $self = shift; - $self->update_index(@_); + my ($self, $biblionums, $records) = @_; + $self->update_index($biblionums, $records, 0); } =head2 $indexer->delete_index($biblionums) @@ -286,7 +285,8 @@ sub _sanitise_records { # tears in rain... if ( $rec ) { $rec->delete_fields($rec->field('999')); - $rec->append_fields(MARC::Field->new('999','','','c' => $bibnum, 'd' => $bibnum)); + # Make sure biblionumber is a string. Elasticsearch would consider int and string different IDs. + $rec->append_fields(MARC::Field->new('999','','','c' => "" . $bibnum, 'd' => "" . $bibnum)); } } } --- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm +++ a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ -102,10 +102,8 @@ sub build_query { if $d && ( $d ne 'asc' && $d ne 'desc' ); $d = 'asc' unless $d; - # TODO account for fields that don't have a 'phrase' type - $f = $self->_sort_field($f); - push @{ $res->{sort} }, { "$f.phrase" => { order => $d } }; + push @{ $res->{sort} }, { $f => { order => $d } }; } } @@ -171,7 +169,7 @@ sub build_browse_query { } } }, - sort => [ { "$sort.phrase" => { order => "asc" } } ], + sort => [ { $sort => { order => "asc" } } ], }; } @@ -294,22 +292,18 @@ sub build_authorities_query { foreach my $s ( @{ $search->{searches} } ) { my ( $wh, $op, $val ) = @{$s}{qw(where operator value)}; $wh = '_all' if $wh eq ''; - if ( $op eq 'is' || $op eq '=' ) { + if ( $op eq 'is' || $op eq '=' || $op eq 'exact' ) { # look for something that matches a term completely # note, '=' is about numerical vals. May need special handling. # Also, we lowercase our search because the ES # index lowercases its values, and term searches don't get the # search analyzer applied to them. - push @query_parts, { term => {"$wh.phrase" => lc $val} }; - } - elsif ( $op eq 'exact' ) { - # left and right truncation, otherwise an exact phrase push @query_parts, { match_phrase => {"$wh.phrase" => lc $val} }; } elsif ( $op eq 'start' ) { # startswith search, uses lowercase untokenized version of heading - push @query_parts, { prefix => {"$wh.lc_raw" => lc $val} }; + push @query_parts, { match_phrase_prefix => {"$wh.phrase" => lc $val} }; } else { # regular wordlist stuff @@ -325,19 +319,17 @@ sub build_authorities_query { # 'should' behaves like 'or' # 'must' behaves like 'and' # Zebra results seem to match must so using that here - my $query = { query=> + my $query = { query => { bool => { must => \@query_parts } } }; - # We need to add '.phrase' to all the sort headings otherwise it'll sort - # based on the tokenised form. my %s; if ( exists $search->{sort} ) { foreach my $k ( keys %{ $search->{sort} } ) { my $f = $self->_sort_field($k); - $s{"$f.phrase"} = $search->{sort}{$k}; + $s{$f} = $search->{sort}{$k}; } $search->{sort} = \%s; } @@ -378,9 +370,9 @@ Also ignored. =item operator -What form of search to do. Options are: is (phrase, no trunction, whole field -must match), = (number exact match), exact (phrase, but with left and right -truncation). If left blank, then word list, right truncted, anywhere is used. +What form of search to do. Options are: is (phrase, no truncation, whole field +must match), = (number exact match), exact (phrase, no truncation, whole field +must match). If left blank, then word list, right truncated, anywhere is used. =item value @@ -412,7 +404,8 @@ our $koha_to_index_name = { 'match-heading' => 'Match-heading', 'see-from' => 'Match-heading-see-from', thesaurus => 'Subject-heading-thesaurus', - all => '' + any => '', + all => '' }; sub build_authorities_query_compat { @@ -441,8 +434,8 @@ sub build_authorities_query_compat { my %sort; my $sort_field = - ( $orderby =~ /^Heading/ ) ? 'Heading__sort' - : ( $orderby =~ /^Auth/ ) ? 'Local-Number' + ( $orderby =~ /^Heading/ ) ? 'Heading' + : ( $orderby =~ /^Auth/ ) ? 'Local-number' : undef; if ($sort_field) { my $sort_order = ( $orderby =~ /Asc$/ ) ? 'asc' : 'desc'; @@ -774,8 +767,18 @@ the end. Maybe it'll be something else in the future, who knows? sub _sort_field { my ($self, $f) = @_; - if ($self->sort_fields()->{$f}) { + + my $mappings = $self->get_elasticsearch_mappings(); + my $textField = defined $mappings->{data}{properties}{$f}{type} && $mappings->{data}{properties}{$f}{type} eq 'text'; + if (!defined $self->sort_fields()->{$f} || $self->sort_fields()->{$f}) { $f .= '__sort'; + # We need to add '.phrase' to text fields, otherwise it'll sort + # based on the tokenised form. + $f .= '.phrase' if $textField; + } else { + # We need to add '.raw' to text fields without a sort field, + # otherwise it'll sort based on the tokenised form. + $f .= '.raw' if $textField; } return $f; } --- a/Koha/SearchEngine/Elasticsearch/Search.pm +++ a/Koha/SearchEngine/Elasticsearch/Search.pm @@ -83,23 +83,21 @@ sub search { my ($self, $query, $page, $count, %options) = @_; my $params = $self->get_elasticsearch_params(); - my %paging; # 20 is the default number of results per page - $paging{limit} = $count || 20; - # ES/Catmandu doesn't want pages, it wants a record to start from. + $query->{size} = $count || 20; + # ES doesn't want pages, it wants a record to start from. if (exists $options{offset}) { - $paging{start} = $options{offset}; + $query->{from} = $options{offset}; } else { $page = (!defined($page) || ($page <= 0)) ? 0 : $page - 1; - $paging{start} = $page * $paging{limit}; + $query->{from} = $page * $query->{size}; } - $self->store( - Catmandu::Store::ElasticSearch->new( - %$params, - ) - ) unless $self->store; + my $elasticsearch = $self->get_elasticsearch(); my $results = eval { - $self->store->bag->search( %$query, %paging ); + $elasticsearch->search( + index => $params->{index_name}, + body => $query + ); }; if ($@) { die $self->process_error($@); @@ -162,13 +160,15 @@ sub search_compat { # opac-search expects results to be put in the # right place in the array, according to $offset my $index = $offset; - $results->each(sub { - $records[$index++] = $self->decode_record_from_result(@_); - }); + my $hits = $results->{'hits'}; + foreach my $es_record (@{$hits->{'hits'}}) { + $records[$index++] = $self->decode_record_from_result($es_record->{'_source'}); + } + # consumers of this expect a name-spaced result, we provide the default # configuration. my %result; - $result{biblioserver}{hits} = $results->total; + $result{biblioserver}{hits} = $hits->{'total'}; $result{biblioserver}{RECORDS} = \@records; return (undef, \%result, $self->_convert_facets($results->{aggregations}, $expanded_facet)); } @@ -176,7 +176,7 @@ sub search_compat { =head2 search_auth_compat my ( $results, $total ) = - $searcher->search_auth_compat( $query, $page, $count, %options ); + $searcher->search_auth_compat( $query, $offset, $count, $skipmetadata, %options ); This has a similar calling convention to L, however it returns its results in a form the same as L. @@ -184,32 +184,39 @@ results in a form the same as L. =cut sub search_auth_compat { - my $self = shift; + my ($self, $query, $offset, $count, $skipmetadata, %options) = @_; - # TODO handle paging + if ( !defined $offset or $offset <= 0 ) { + $offset = 1; + } + # Uh, authority search uses 1-based offset.. + $options{offset} = $offset - 1; my $database = Koha::Database->new(); my $schema = $database->schema(); - my $res = $self->search(@_); + my $res = $self->search($query, undef, $count, %options); + my $bib_searcher = Koha::SearchEngine::Elasticsearch::Search->new({index => 'biblios'}); my @records; - $res->each( - sub { - my %result; + my $hits = $res->{'hits'}; + foreach my $es_record ($hits->{'hits'}) { + my $record = $es_record->[0]->{'_source'}; + my %result; - # I wonder if these should be real values defined in the mapping - # rather than hard-coded conversions. - my $record = $_[0]; - # Handle legacy nested arrays indexed with splitting enabled. - my $authid = $record->{ 'Local-number' }[0]; - $authid = @$authid[0] if (ref $authid eq 'ARRAY'); + # I wonder if these should be real values defined in the mapping + # rather than hard-coded conversions. + #my $record = $_[0]; + # Handle legacy nested arrays indexed with splitting enabled. + my $authid = $record->{ 'Local-number' }[0]; + $authid = @$authid[0] if (ref $authid eq 'ARRAY'); - $result{authid} = $authid; + $result{authid} = $authid; + if (!defined $skipmetadata || !$skipmetadata) { # TODO put all this info into the record at index time so we # don't have to go and sort it all out now. my $authtypecode = $record->{authtype}; my $rs = $schema->resultset('AuthType') - ->search( { authtypecode => $authtypecode } ); + ->search( { authtypecode => $authtypecode } ); # FIXME there's an assumption here that we will get a result. # the original code also makes an assumption that some provided @@ -218,7 +225,7 @@ sub search_auth_compat { # it's not reproduced here yet. my $authtype = $rs->single; my $auth_tag_to_report = $authtype ? $authtype->auth_tag_to_report : ""; - my $marc = $self->decode_record_from_result(@_); + my $marc = $self->decode_record_from_result($record); my $mainentry = $marc->field($auth_tag_to_report); my $reported_tag; if ($mainentry) { @@ -232,13 +239,13 @@ sub search_auth_compat { # Reimplementing BuildSummary is out of scope because it'll be hard $result{summary} = - C4::AuthoritiesMarc::BuildSummary( $marc, $result{authid}, + C4::AuthoritiesMarc::BuildSummary( $marc, $result{authid}, $authtypecode ); $result{used} = $self->count_auth_use($bib_searcher, $authid); - push @records, \%result; } - ); - return ( \@records, $res->total ); + push @records, \%result; + } + return ( \@records, $hits->{'total'} ); } =head2 count_auth_use --- a/Koha/SearchEngine/Zebra/Search.pm +++ a/Koha/SearchEngine/Zebra/Search.pm @@ -100,12 +100,12 @@ This passes the search query on to C4::AuthoritiesMarc::SearchAuthorities =cut sub search_auth_compat { - my ( $self, $q, $startfrom, $resperpage ) = @_; + my ( $self, $q, $startfrom, $resperpage, $skipmetadata ) = @_; my @params = ( @{$q}{ 'marclist', 'and_or', 'excluding', 'operator', 'value' }, $startfrom - 1, - $resperpage, @{$q}{ 'authtypecode', 'orderby' } + $resperpage, @{$q}{ 'authtypecode', 'orderby' }, $skipmetadata ); C4::AuthoritiesMarc::SearchAuthorities(@params); } --- a/misc/link_bibs_to_authorities.pl +++ a/misc/link_bibs_to_authorities.pl @@ -195,9 +195,10 @@ sub process_bib { return; } + my $frameworkcode = GetFrameworkCode($biblionumber); + my ( $headings_changed, $results ) = - LinkBibHeadingsToAuthorities( $linker, $bib, - GetFrameworkCode($biblionumber) ); + LinkBibHeadingsToAuthorities( $linker, $bib, $frameworkcode ); foreach my $key ( keys %{ $results->{'unlinked'} } ) { $unlinked_headings{$key} += $results->{'unlinked'}->{$key}; } @@ -211,11 +212,15 @@ sub process_bib { if ($headings_changed) { if ($verbose) { my $title = substr( $bib->title, 0, 20 ); - print -"Bib $biblionumber ($title): $headings_changed headings changed\n"; + printf( + "Bib %12d (%-20s): %3d headings changed\n", + $biblionumber, + $title, + $headings_changed + ); } if ( not $test_only ) { - ModBiblio( $bib, $biblionumber, GetFrameworkCode($biblionumber) ); + ModBiblio( $bib, $biblionumber, $frameworkcode ); $num_bibs_modified++; } } --