|
Description
Galen Charlton
2013-10-24 20:16:04 UTC
Created attachment 22380 [details] [review] Bug 11137: regression tests for QP search field alias bug This patch adds regression tests to verify that Boolean searches using QueryParser function correctly. This patch also ensures that QP is correctly initalized when Search.t is run. To test: [1] Apply this patch and the following patch. [2] Verify that prove -v t/QueryParser.t works [3] Verify that prove -v t/db_dependent/Search.t works [4] (optional) instead of applying both patches at the same time, apply only the regression test patch and run the tests listed in steps 2 and 3. The following tests should fail: t/db_dependent/Search.t (Wstat: 512 Tests: 198 Failed: 2) Failed tests: 42, 71 Non-zero exit status: 2 t/QueryParser.t (Wstat: 256 Tests: 28 Failed: 1) Failed test: 12 Non-zero exit status: 1 Signed-off-by: Galen Charlton <gmc@esilibrary.com> Created attachment 22381 [details] [review] Bug 11137: fix certain types of searches run using QueryParser QueryParser was loading redundant search aliases, which in turn resulted in certain queries involving two or more clauses joined by a Boolean operator getting parsed in correctly. To test: [1] Enable QueryParser. [2] Arrange your database so that some some bib records contain the keyword 'history' while a smaller subset contain both 'history' and 'earth'. (The exact words used don't matter.) [3] Run the following search from the OPAC global search bar: kw:history && kw:earth [4] Observe that the records that are returned include *all* bibs in the database with the keyword 'history'. In other words, the restriction that the records must also contain 'earth' is not observed. [5] Apply the patch and rerun the search. [6] This time, the results that are returned should include only those that contain both 'history' and 'earth'. [7] Verify that prove -v t/QueryParser.t passes. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Created attachment 22383 [details] [review] Bug 11137: regression tests for QP search field alias bug This patch adds regression tests to verify that Boolean searches using QueryParser function correctly. This patch also ensures that QP is correctly initalized when Search.t is run. To test: [1] Apply this patch and the following patch. [2] Verify that prove -v t/QueryParser.t works [3] Verify that prove -v t/db_dependent/Search.t works [4] (optional) instead of applying both patches at the same time, apply only the regression test patch and run the tests listed in steps 2 and 3. The following tests should fail: t/db_dependent/Search.t (Wstat: 512 Tests: 198 Failed: 2) Failed tests: 42, 71 Non-zero exit status: 2 t/QueryParser.t (Wstat: 256 Tests: 28 Failed: 1) Failed test: 12 Non-zero exit status: 1 Signed-off-by: Galen Charlton <gmc@esilibrary.com> Created attachment 22384 [details] [review] Bug 11137: fix certain types of searches run using QueryParser QueryParser was loading redundant search aliases, which in turn resulted in certain queries involving two or more clauses joined by a Boolean operator getting parsed in correctly. To test: [1] Enable QueryParser. [2] Arrange your database so that some some bib records contain the keyword 'history' while a smaller subset contain both 'history' and 'earth'. (The exact words used don't matter.) [3] Run the following search from the OPAC global search bar: kw:history && kw:earth [4] Observe that the records that are returned include *all* bibs in the database with the keyword 'history'. In other words, the restriction that the records must also contain 'earth' is not observed. [5] Apply the patch and rerun the search. [6] This time, the results that are returned should include only those that contain both 'history' and 'earth'. [7] Verify that prove -v t/QueryParser.t passes. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Created attachment 22438 [details] [review] [SIGNED-OFF] Bug 11137: regression tests for QP search field alias bug This patch adds regression tests to verify that Boolean searches using QueryParser function correctly. This patch also ensures that QP is correctly initalized when Search.t is run. To test: [1] Apply this patch and the following patch. [2] Verify that prove -v t/QueryParser.t works [3] Verify that prove -v t/db_dependent/Search.t works [4] (optional) instead of applying both patches at the same time, apply only the regression test patch and run the tests listed in steps 2 and 3. The following tests should fail: t/db_dependent/Search.t (Wstat: 512 Tests: 198 Failed: 2) Failed tests: 42, 71 Non-zero exit status: 2 t/QueryParser.t (Wstat: 256 Tests: 28 Failed: 1) Failed test: 12 Non-zero exit status: 1 Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 22439 [details] [review] [SIGNED-OFF] Bug 11137: fix certain types of searches run using QueryParser QueryParser was loading redundant search aliases, which in turn resulted in certain queries involving two or more clauses joined by a Boolean operator getting parsed in correctly. To test: [1] Enable QueryParser. [2] Arrange your database so that some some bib records contain the keyword 'history' while a smaller subset contain both 'history' and 'earth'. (The exact words used don't matter.) [3] Run the following search from the OPAC global search bar: kw:history && kw:earth [4] Observe that the records that are returned include *all* bibs in the database with the keyword 'history'. In other words, the restriction that the records must also contain 'earth' is not observed. [5] Apply the patch and rerun the search. [6] This time, the results that are returned should include only those that contain both 'history' and 'earth'. [7] Verify that prove -v t/QueryParser.t passes. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes test plans, all tests and QA script. Also checked the db_dependent/Search.t tests. Tried some other searches, all seems to be working correctly. I cannot test this patch. My MARC21 DB is broken and with my UNIMARC DB, I got no result. I tried to debug: In QueryParser::Driver::PQF::target_syntax, debug is enable. - with marcflavour=MARC21 QP query for biblioserver: history #relevance_dsc PQF query: @attr 2=102 @or @or @attr 1=1016 @attr 5=1 @attr 4=6 "history" @attr 9=20 @attr 2=102 @attr 5=1 @attr 4=6 "history" @attr 9=34 @attr 2=102 @attr 5=1 @attr 4=6 "history" - with marcflavour=UNIMARC QP query for biblioserver: history #relevance_asc PQF query: @and @or @or @attr 1=1016 @attr 4=6 "history" @attr 9=20 @attr 2=102 @attr 4=6 "history" @attr 9=34 @attr 2=102 @attr 4=6 "history" @or @or @attr 1=1016 @attr 4=6 "#relevance_asc" @attr 9=20 @attr 2=102 @attr 4=6 "#relevance_asc" @attr 9=34 @attr 2=102 @attr 4=6 "#relevance_asc" The call to $self->parse_tree->target_syntax does not remove the modifiers for UNIMARC. I don't know how continue to investigate. So stupid... My Unimarc DB got a default sort field relevance dsc and there is no modifier relevance dsc in the qp.yml file. Created attachment 22932 [details] [review] Bug 11137: regression tests for QP search field alias bug This patch adds regression tests to verify that Boolean searches using QueryParser function correctly. This patch also ensures that QP is correctly initalized when Search.t is run. To test: [1] Apply this patch and the following patch. [2] Verify that prove -v t/QueryParser.t works [3] Verify that prove -v t/db_dependent/Search.t works [4] (optional) instead of applying both patches at the same time, apply only the regression test patch and run the tests listed in steps 2 and 3. The following tests should fail: t/db_dependent/Search.t (Wstat: 512 Tests: 198 Failed: 2) Failed tests: 42, 71 Non-zero exit status: 2 t/QueryParser.t (Wstat: 256 Tests: 28 Failed: 1) Failed test: 12 Non-zero exit status: 1 Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 22933 [details] [review] Bug 11137: fix certain types of searches run using QueryParser QueryParser was loading redundant search aliases, which in turn resulted in certain queries involving two or more clauses joined by a Boolean operator getting parsed in correctly. To test: [1] Enable QueryParser. [2] Arrange your database so that some some bib records contain the keyword 'history' while a smaller subset contain both 'history' and 'earth'. (The exact words used don't matter.) [3] Run the following search from the OPAC global search bar: kw:history && kw:earth [4] Observe that the records that are returned include *all* bibs in the database with the keyword 'history'. In other words, the restriction that the records must also contain 'earth' is not observed. [5] Apply the patch and rerun the search. [6] This time, the results that are returned should include only those that contain both 'history' and 'earth'. [7] Verify that prove -v t/QueryParser.t passes. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes test plans, all tests and QA script. Also checked the db_dependent/Search.t tests. Tried some other searches, all seems to be working correctly. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> I reproduce the bug and I confirm this patch fixes it. Without the patch, kw:history && kw:earth, I got: Search biblios OK 4 1 1+0 RPN @attrset Bib-1 @attr 4=6 @attr 1=1016 history With the path, I get: Search biblios OK 2 1 1+0 RPN @attrset Bib-1 @and @attr 4=6 @attr 1=1016 history @attr 4=6 @attr 1=1016 earth Galen: I know there is no sense to sort by relevance asc but don't you know it is worth to add an entry in the qp config file? (In reply to Jonathan Druart from comment #11) > Galen: I know there is no sense to sort by relevance asc but don't you know > it is worth to add an entry in the qp config file? I think so, as there's nothing preventing somebody from setting relevance ascending as the default sort order. Pushed to master and 3.14.x. (In reply to Galen Charlton from comment #12) > (In reply to Jonathan Druart from comment #11) > > Galen: I know there is no sense to sort by relevance asc but don't you know > > it is worth to add an entry in the qp config file? > > I think so, as there's nothing preventing somebody from setting relevance > ascending as the default sort order. Patch submitted in bug 11255. This patch has been pushed to 3.12.x, will be in 3.12.8. Thanks Galen! |