Bug 12443 - Initial re-factoring of buildQuery
Summary: Initial re-factoring of buildQuery
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12689
  Show dependency treegraph
 
Reported: 2014-06-18 05:35 UTC by David Cook
Modified: 2015-06-04 23:33 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12443 - Initial re-factoring of buildQuery (4.74 KB, patch)
2014-06-18 07:02 UTC, David Cook
Details | Diff | Splinter Review
Bug 12443 - Initial re-factoring of buildQuery (5.17 KB, patch)
2014-06-18 07:05 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12443 - Initial re-factoring of buildQuery (5.27 KB, patch)
2014-06-18 12:56 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 12443 - Initial re-factoring of buildQuery (5.32 KB, patch)
2014-06-20 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12443: (QA followup) Unit tests for _build_initial_query (6.81 KB, patch)
2014-06-26 18:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2014-06-18 05:35:01 UTC
Before I start trying to fix bad/broken behaviour in buildQuery, I want to refactor it to preserve the current behaviour but in a form that will be easier to fix later.

The goal of this bug will be to normalize how the parsed query parameters are formed into the actual Zebra query.

Actually a lot more straightforward than it sounds...
Comment 1 David Cook 2014-06-18 07:02:00 UTC Comment hidden (obsolete)
Comment 2 David Cook 2014-06-18 07:05:54 UTC Comment hidden (obsolete)
Comment 3 David Cook 2014-06-18 07:07:22 UTC
You should probably turn off TryQueryParser when doing testing as well.

While QueryParser sometimes falls back to non-QueryParser, it's just easier if you turn it off explicitly for testing purposes.
Comment 4 Bernardo Gonzalez Kriegel 2014-06-18 12:56:39 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2014-06-20 16:41:27 UTC
Created attachment 29116 [details] [review]
[PASSED QA] Bug 12443 - Initial re-factoring of buildQuery

This patch reduces three repeated code fragments into a single
internal subroutine, which is easier to read, has comments,
and should make it easier to refactor more buildQuery code
in the future.

_TEST PLAN_

Before applying

1) Run a bunch of different searches in the staff client and OPAC
in separate tabs

2) Apply the patch

3) Run the same searches again (maybe in yet more tabs) and notice
that the results are exactly the same.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Same results, no errors.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Tomás Cohen Arazi 2014-06-21 13:45:54 UTC
I'd like to see some unit tests for such a sensitive core functionality.
I know it is an almost trivial change, but we are introducing a new function to C4::Search which needs to be documented (ut-driven documentation).

It does, OTOH, look fine to me and would push it soon. If help is needed on writing those tests don't hesitate to write me.
Comment 7 David Cook 2014-06-23 03:19:00 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I'd like to see some unit tests for such a sensitive core functionality.
> I know it is an almost trivial change, but we are introducing a new function
> to C4::Search which needs to be documented (ut-driven documentation).
> 
> It does, OTOH, look fine to me and would push it soon. If help is needed on
> writing those tests don't hesitate to write me.

I was thinking about unit tests when I wrote the patch, but I'm not really sure what sort of test would be appropriate for this type of function. 

I can write the test, but I don't know what to test, if that makes sense.
Comment 8 Tomás Cohen Arazi 2014-06-23 18:02:06 UTC
(In reply to David Cook from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > I'd like to see some unit tests for such a sensitive core functionality.
> > I know it is an almost trivial change, but we are introducing a new function
> > to C4::Search which needs to be documented (ut-driven documentation).
> > 
> > It does, OTOH, look fine to me and would push it soon. If help is needed on
> > writing those tests don't hesitate to write me.
> 
> I was thinking about unit tests when I wrote the patch, but I'm not really
> sure what sort of test would be appropriate for this type of function. 
> 
> I can write the test, but I don't know what to test, if that makes sense.

You could start by looking at t/db_dependent/Search.t for stuff like this:

<code>
( $error, $query, $simple_query, $query_cgi,
 $query_desc, $limit, $limit_cgi, $limit_desc,
 $stopwords_removed, $query_type ) = buildQuery([ 'and' ], [ 'salud', 'higiene' ], [], [], [], 0, 'en');
    like($query, qr/kw\W.*salud\W.*and.*kw\W.*higiene/, "Built composed explicit-and CCL keyword query");
</code>

And build scenarios that cover all conditional paths.
Comment 9 David Cook 2014-06-23 23:25:28 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> (In reply to David Cook from comment #7)
> > (In reply to Tomás Cohen Arazi from comment #6)
> > > I'd like to see some unit tests for such a sensitive core functionality.
> > > I know it is an almost trivial change, but we are introducing a new function
> > > to C4::Search which needs to be documented (ut-driven documentation).
> > > 
> > > It does, OTOH, look fine to me and would push it soon. If help is needed on
> > > writing those tests don't hesitate to write me.
> > 
> > I was thinking about unit tests when I wrote the patch, but I'm not really
> > sure what sort of test would be appropriate for this type of function. 
> > 
> > I can write the test, but I don't know what to test, if that makes sense.
> 
> You could start by looking at t/db_dependent/Search.t for stuff like this:
> 
> <code>
> ( $error, $query, $simple_query, $query_cgi,
>  $query_desc, $limit, $limit_cgi, $limit_desc,
>  $stopwords_removed, $query_type ) = buildQuery([ 'and' ], [ 'salud',
> 'higiene' ], [], [], [], 0, 'en');
>     like($query, qr/kw\W.*salud\W.*and.*kw\W.*higiene/, "Built composed
> explicit-and CCL keyword query");
> </code>
> 
> And build scenarios that cover all conditional paths.

Since I'm just changing the internals of buildQuery, shouldn't the existing plans cover that though?
Comment 10 Tomás Cohen Arazi 2014-06-26 18:46:04 UTC
Created attachment 29300 [details] [review]
Bug 12443: (QA followup) Unit tests for _build_initial_query

This followup adds uint tests for the newly introduced function.
It is really basic, but covers all scenarios for the new function,
and will help us prevent regressions in future patches.

Regards
To+

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 11 Tomás Cohen Arazi 2014-06-27 11:52:59 UTC
Patch pushed to master.

Thanks David!
Comment 12 David Cook 2014-06-30 08:33:55 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> Patch pushed to master.
> 
> Thanks David!

Thanks, Tomás.

I'm thinking of adding some "See Also" bugs for outstanding Search issues to fix in future rather than creating a new bugzilla issue as an omnibus bug. 

Off the top of my head, I'm planning to resolve existing issues with truncation and weighted queries which are causing all sorts of problems.

I'll be sure to write unit tests for them as well. Thanks for adding these examples. They give me ideas about how to test my future code :D.
Comment 13 David Cook 2014-07-02 02:53:30 UTC
PSA on truncation with Zebra:

It appears that the @truncation directive in ccl.properties doesn't work in versions before Yaz 4.2.21 (going back to an indeterminate time).

"yaz-client -V" will tell you what version of Yaz you have.

Debian Squeeze uses Yaz 4.0.11-1
Debian Wheezy uses Yaz 4.2.30

So...if you're on Wheezy, you're probably fine. If you're on Squeeze, your @truncation directive isn't working, which means the truncation character is the default "?" no matter what you put in ccl.properties.

It does appear that you can provide multiple truncation characters based on the reference to "truncation_aliases" in the following code though:
http://www.indexdata.com/yaz/dox/html/cclfind_8c_source.html

--- 4.2.21 2011/11/23

Fix cql_sortby_to_sortkeys. Completely broken before.

Make CCL's @truncation work again.

Source: http://www.indexdata.com/yaz/doc/NEWS
Comment 14 David Cook 2014-07-02 07:14:58 UTC
More thoughts on  C4::Search::buildQuery and Koha::QueryParser...


C4::Search::buildQuery:

PRO: Right/left/right&&left truncation, QueryFuzzy appears to work, QueryWeightFields works (except when QueryAutoTruncate is on), QueryStemming works with wordlists.

CON: No support for phrases, lots of ugly spaghetti code which is mutually exclusive, incorrectly defines variables, etc.

FUTURE:
Planning to integrate "QueryWeightFields", "QueryFuzzy", "QueryAutoTruncate", and "QueryStemming" into _build_initial_query().
"QueryWeightFields" and "QueryFuzzy" could be improved.

"QueryStemming" is a tough one...as it really is only useful for Word Lists...so we'll have to check the structure and make sure there are no quotation marks sneaking in as they'll cause problems. That should work all right.

--


Koha::QueryParser:

PRO: Automatically detects phrases, automatically detects right truncation.

"Koha::QueryParser::Driver::PQF::query_plan::node" already uses "QueryAutoTruncate", some "QueryWeightFields"-like functionality (which needs a more configuration and possibly some code tweaks).

CON: No left truncation, no left && right truncation, no easy way to designate a phrase (except by wrapping in quotes or by providing an alias)

FUTURE: "Koha::QueryParser::Driver::PQF::query_plan::node" could make it easy to add "QueryStemming" and "QueryFuzzy". We could also add support for left truncation and left && right truncation using that same module. 

Improve relevance bumps (aka QueryWeightFields functionality) in queryparser.yaml.
Comment 15 Robin Sheat 2014-07-25 04:02:23 UTC
The test case fails to build on Debian testing with:

String found where operator expected at t/Search.t line 28, near "subtest "_build_initial_query tests""
	(Do you need to predeclare subtest?)
syntax error at t/Search.t line 28, near "subtest "_build_initial_query tests""
syntax error at t/Search.t line 158, near "}"
Execution of t/Search.t aborted due to compilation errors.
Comment 16 David Cook 2014-07-25 04:29:06 UTC
(In reply to Robin Sheat from comment #15)
> The test case fails to build on Debian testing with:
> 
> String found where operator expected at t/Search.t line 28, near "subtest
> "_build_initial_query tests""
> 	(Do you need to predeclare subtest?)
> syntax error at t/Search.t line 28, near "subtest "_build_initial_query
> tests""
> syntax error at t/Search.t line 158, near "}"
> Execution of t/Search.t aborted due to compilation errors.

Sounds like Debian testing uses an older version of Test::More than Tomas, me thinks. 

Same issue below, I imagine:

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9142
Comment 17 Tomás Cohen Arazi 2014-07-25 12:34:39 UTC
(In reply to David Cook from comment #16)
> (In reply to Robin Sheat from comment #15)
> > The test case fails to build on Debian testing with:
> > 
> > String found where operator expected at t/Search.t line 28, near "subtest
> > "_build_initial_query tests""
> > 	(Do you need to predeclare subtest?)
> > syntax error at t/Search.t line 28, near "subtest "_build_initial_query
> > tests""
> > syntax error at t/Search.t line 158, near "}"
> > Execution of t/Search.t aborted due to compilation errors.
> 
> Sounds like Debian testing uses an older version of Test::More than Tomas,
> me thinks. 
> 
> Same issue below, I imagine:
> 
> http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9142

I don't think it is the same problem, as we have subtests in:

db_dependent/Koha.t
db_dependent/Biblio.t
db_dependent/Items.t
db_dependent/Search.t
Search.t

and they are not failing in Debian 6, Debian 7, Ubuntu 12.04 and Ubuntu 14.04.
Can anyone try those on Debian testing please? Also, we definitely would benefit of a Debian testing node.
Comment 18 Robin Sheat 2014-07-25 21:59:57 UTC
When I said testing, I meant squeeze. I forgot that I build it inside a chroot.
Comment 19 Robin Sheat 2014-07-28 04:06:38 UTC
FYI, it fails in my squeeze chroot, but it passes in testing.
Comment 20 Robin Sheat 2014-07-31 23:43:20 UTC
OK, I'm going to have to make another bug from this, it's currently blocking build of master, which isn't good, and I don't know how to fix it properly.