Description
Ere Maijala
2019-03-27 12:27:27 UTC
Created attachment 87135 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Created attachment 87136 [details] [review] Bug 22592: Fixes to index scan Makes sure the scan term is displayed and redirection to record page is not used with index scan for a single result. Also adds a submit button so that you can submit a change for selected index or term easier. I can't apply this patch. Do I need anything other than 22413 to make this work? It fails with: Applying: Bug 22592: Add index scan emulation to Elasticsearch error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm). error: could not build fake ancestor Patch failed at 0001 Bug 22592: Add index scan emulation to Elasticsearch Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-22592-Add-index-scan-emulation-to-Elasticsearc-iqm9C3.patch Created attachment 87344 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Created attachment 87345 [details] [review] Bug 22592: Fixes to index scan Makes sure the scan term is displayed and redirection to record page is not used with index scan for a single result. Also adds a submit button so that you can submit a change for selected index or term easier. Stefan, thanks for testing. Nothing else is needed. I had to rebase, though, since there were changes in master. Just let bz follow the dependency and you should be good (for now...). Hi Ere, I'm not sure what but something make Elastic broken. I can!t reidex ES, U get: Can't locate object method "throw" via package "Koha::Exceptions::Elasticsearch::MARCFieldExprParseError" (perhaps you forgot to load "Koha::Exceptions::Elasticsearch::MARCFieldExprParseError"?) at /home/vagrant/kohaclone/Koha/SearchEngine/Elasticsearch.pm line 794. I followed all dependencies. Hi Ere, I'm not sure what but something make Elastic broken. I can!t reidex ES, U get: Can't locate object method "throw" via package "Koha::Exceptions::Elasticsearch::MARCFieldExprParseError" (perhaps you forgot to load "Koha::Exceptions::Elasticsearch::MARCFieldExprParseError"?) at /home/vagrant/kohaclone/Koha/SearchEngine/Elasticsearch.pm line 794. I followed all dependencies. Sorry, T was some problem with my kohadevbox. I set Need signet off back. Created attachment 87668 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87669 [details] [review] Bug 22592: Fixes to index scan Makes sure the scan term is displayed and redirection to record page is not used with index scan for a single result. Also adds a submit button so that you can submit a change for selected index or term easier. Signed-off-by: Michal Denar <black23@gmail.com> Patch does not apply Applying: Bug 22592: Add index scan emulation to Elasticsearch fatal: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch/QueryBuilder.pm). error: could not build fake ancestor Patch failed at 0001 Bug 22592: Add index scan emulation to Elasticsearch The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-22592-Add-index-scan-emulation-to-Elasticsearc-ykcnua.patch Created attachment 88777 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88778 [details] [review] Bug 22592: Fixes to index scan Makes sure the scan term is displayed and redirection to record page is not used with index scan for a single result. Also adds a submit button so that you can submit a change for selected index or term easier. Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88807 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88808 [details] [review] Bug 22592: Fixes to index scan Makes sure the scan term is displayed and redirection to record page is not used with index scan for a single result. Also adds a submit button so that you can submit a change for selected index or term easier. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> 1) This feature is MARC21 specific. Doesn't work with UNIMARC. 2) Typo in the POD for _build_scan_query and _create_regex_filter (missing underscores) 3) Missing tests for _aggregation_scan / search_compat 4) 4 tests added in QueryBuilder.t but subtest's plan test not updated. Tests fail. Created attachment 92391 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 92392 [details] [review] Bug 22592: Fixes to index scan Makes sure the scan term is displayed and redirection to record page is not used with index scan for a single result. Also adds a submit button so that you can submit a change for selected index or term easier. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 92393 [details] [review] Bug 22592: Rebase, add UNIMARC support and fix tests. I got to it too late - can you please rebase? Created attachment 93743 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Rebased and squashed the commits. In addition there is one change to search.pl to make it only consider the first idx parameter for the scan index to use. For some reason this check was commented out since the introduction of the code block in commit https://github.com/Koha-Community/Koha/commit/9f3e6b4a407ff79a5fa9ff627077dee884bccc24, but without it the searches initiated from a scan field will use wrong search index. That's 11 years ago, which makes me wonder if something else broke it more recently, since otherwise it just means nobody is using the feature. ;) Hi Ere, I've just tested with a sandbox and have an issue with the "Scan index" count, which is different of the total of results I get if I perform the search. I wondered about comment 17 made by Alex : is it supposed to work with Unimarc ? Is it the origin of my issue ? Thanks ! The search it performs is currently not quite perfect match for the term list since search ignores punctuation etc. and may include results that start with the given term. Since the created search query does not carry information on the actual origin of the search, we can't handle it in a different way from a normal search in the ES code. I'm sure this could be further refined, but this is a minimal implementation to make not just fail completely. So if my understanding is good, you mean I can sign the patch, am I right ? :) Yes, exactly! :) Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr> Created attachment 93745 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> (In reply to Ere Maijala from comment #27) > Yes, exactly! :) Thank you Ere ! Created attachment 94962 [details] [review] Bug 22592: Add index scan emulation to Elasticsearch Adds support for using the "scan indexes" action in advanced search by using faceting with a prefix filter. Requires that the field be set as facetable for anything to be found. Test plan: 1. Apply patch 2. Go to advanced search and click "More options" 3. Select author as the search field, enter a last name and check "Scan indexes" 4. Perform search and observe the result list resembling scan results Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I feel while this is not the 'perfect' scan search, it's a great first step forwards from not supporting it to giving pretty good results in most cases (better than with Zebra actually). I think we could even discuss for this to be a bug fix, as we offer the scan search with Elastic, but it didn't work before. Nice work! Pushed to master for 19.11.00 |