do_search refactoring: performance, legibility, move validation to validation/submit handlers of form.

To make the performance aspect clear, here is the count query before and after this patch:

# Before

SELECT COUNT(*) FROM (
  SELECT i.type, i.sid, 
         (5 * COALESCE((2.0 - 2.0 / (1.0 + node_comment_statistics.comment_count * 0.00271002710027)), 0) + 
          5 * COALESCE(((34.5707888161 * SUM(i.score * t.count))), 0) + 
          5 * COALESCE((n.sticky), 0) + 
          5 * COALESCE((n.promote), 0) + 
          5 * COALESCE(((POW(2, GREATEST(n.created, n.changed) - 1072477462) * 6.43e-8)), 0)) AS score 
    FROM search_index i 
    INNER JOIN search_total t ON i.word = t.word 
    INNER JOIN node n ON n.nid = i.sid 
    LEFT JOIN node_comment_statistics node_comment_statistics ON node_comment_statistics.nid = i.sid 
    WHERE n.status = 1 AND (i.word = 'dries') AND i.type = 'node' 
    GROUP BY i.type, i.sid HAVING COUNT(*) >= 1) n1

# After

SELECT COUNT(*) FROM (
  SELECT 1 FROM search_index i 
  INNER JOIN node n ON n.nid = i.sid 
  WHERE n.status = 1 AND (i.word = 'dries') AND i.type = 'node' 
  GROUP BY i.type, i.sid HAVING COUNT(*) >= 1) n1  

Oops, looks like I lost part of the patch. Need to try and recover and merge.

This patch includes the changes to the search box form and the search form submit handler which are important for keeping do_search out of the input validation business.

Still managed to miss a bit. This version has it all.

I've tested this again and it still applies, still improves the queries and legibility of the search code. Please review #8.

I'm also getting warnings when submitting from the search block instead of the search form.

# notice: Undefined index: basic in ../drupal-7/modules/search/search.pages.inc on line 114.
# notice: Undefined index: keys in .../drupal-7/modules/search/search.pages.inc on line 114.
# warning: array_shift() [function.array-shift]: The argument should be an array in .../drupal-7/includes/form.inc on line 1308.

These warnings are dealt with here: http://drupal.org/node/280319

They're unrelated to this patch.

Rerolled to reflect http://drupal.org/cvs?commit=126469

Changed "positive keyword" to "keyword".

Ran tests, did some taste testing manually. Looks ok.

The word "positive" stayed in with that last one.

I've written tests that check whether the advanced search works. In particular I test the type option. It would be good to add tests to test vocabulary options, language options, etc., but at least I solved the problem of how to test the advanced search form (was a non-trivial effort). I also adjusted do_search so that it is now passing all of the tests. I don't have full confidence in its correctness, though, and would love for others to do some more testing, and perhaps follow the model I set up in the test to test other parts of the advanced search form.

Yes, please review what is there. Eventually someone needs to add the other tests, but your reviews are very helpful. Thank you.

Re-roll. Tests are broken.

Some tests still failing.

Tests pass.

And now for the cool part. It's not in the patch yet. The count query (and I think the search query) can be simplified (for the search query "dolore -sit") in the cases of phrase queries from this:

SELECT COUNT(*) FROM (
  SELECT count(*) FROM 
    search_dataset d 
    INNER JOIN node n ON n.nid = d.sid 
    WHERE n.status = 1 AND 
    d.type = 'node' AND 
    (d.data LIKE '% dolore %' AND 
    d.data NOT LIKE '% sit %') 
    GROUP BY d.type, d.sid 
    HAVING COUNT(*) >= 1
  ) n1;

to this:

SELECT count(*) 
  FROM search_dataset d 
  INNER JOIN node n ON n.nid = d.sid 
  WHERE n.status = 1 AND 
    d.type = 'node' AND 
    (d.data LIKE '% dolore %' AND 
     d.data NOT LIKE '% sit %');

So leaving marked as "needs work" because there is still so much that can be done here.

Furthermore, in some cases, the HAVING clause is not needed to get the right result. This needs more investigation:

mysql> SELECT COUNT(*) FROM (SELECT 1 FROM search_index i INNER JOIN node n ON n.nid = i.sid WHERE n.status = 1 AND (i.word = 'dolore') AND i.type = 'node' GROUP BY i.type, i.sid HAVING COUNT(*) >= 1) n1;
+----------+
| COUNT(*) |
+----------+
|        3 | 
+----------+
1 row in set (0.00 sec)

mysql> SELECT COUNT(*) FROM (SELECT 1 FROM search_index i INNER JOIN node n ON n.nid = i.sid WHERE n.status = 1 AND (i.word = 'dolore') AND i.type = 'node' GROUP BY i.type, i.sid) n1;
+----------+
| COUNT(*) |
+----------+
|        3 | 
+----------+
1 row in set (0.00 sec)

I'm testing right now to find out if the query variant without the search_index table is a good idea. It probably causes full table scans which is what Steven wanted to avoid, thus the original structure of the query. Will report.

Benchmarks answered the question in #29: the shorter version of the query is faster, and markedly so. I tested three different queries of differing complexity, and just used PHP to execute the database query a number of times. In all cases the shorter query was faster and produced identical results. Here is an example of two of the queries I used, and the results.

SELECT COUNT(*) FROM (
  SELECT 1 FROM search_index i 
    INNER JOIN node n ON n.nid = i.sid 
    INNER JOIN search_dataset d ON i.sid = d.sid AND i.type = d.type 
  WHERE n.status = 1 AND 
    (i.word = 'genitus' OR i.word = 'ratis' OR i.word = 'abigo' OR i.word = 'natu' OR i.word = 'ideo') AND 
    i.type = 'node' AND 
    (d.data LIKE '% os genitus ratis abigo natu ideo %') 
  GROUP BY i.type, i.sid 
  HAVING COUNT(*) >= 5) n1;    

SELECT COUNT(*) FROM search_dataset d 
  INNER JOIN node n ON n.nid = d.sid  
  WHERE n.status = 1 AND 
    d.type = 'node' AND 
    (d.data LIKE '% os genitus ratis abigo natu ideo %');

Here's the patch that uses the new query structure for count queries. I haven't gotten to using it for search queries (and haven't seen whether it is possible). However, a certain type of tests fail.

The tests that fail seem to fail for reasons that are only documented in the code comments of the unit tests:

  /**
   * Run predefine queries looking for indexed terms.
   */
  function _testQueries() {
    /*
      Note: OR queries that include short words in OR groups are only accepted
      if the ORed terms are ANDed with at least one long word in the rest of the query.

      e.g. enim dolore OR ut = enim (dolore OR ut) = (enim dolor) OR (enim ut) -> good
      e.g. dolore OR ut = (dolore) OR (ut) -> bad

      This is a design limitation to avoid full table scans.
    */

Even though the tests fail, searching for the same terms in a normal website context works. So I don't know what's going on and have to investigate more.

I've moved the test to a separate issue in the hopes that making a smaller patch will help people review. The test is now here: http://drupal.org/node/308233

Tests pass in this. But don't rejoice! It's only because of an ugly hack that proves the concept and also proves that the signature of do_search has to be refactored. Basically it comes down to this: passing in snippets of SQL to inject into queries is too fragile. Since the technique of this patch is to remove the search_index table from the query in some cases, the node module can no longer assume that the search_index i part of the query will be there. So we need to move to a more meta-data driven query builder (like the views query builder!!!)