This issue is part of #2454513: [meta] Make Drupal 8 work with SQLite.

The test case Drupal\search\Tests\SearchRankingTest is currently failing on SQLite.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

amateescu’s picture

Status: Active » Needs review
FileSize
4.03 KB

This test exposes multiple problems in the search module:

  1. Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1 ambiguous column name: sid
    

    This happens because the 'recent' ranking defined in node_ranking() adds a LEFT JOIN on the node_field_data table without specifying the table alias for the search_index sid column. But Drupal\node\Plugin\Search\NodeSearch::findResults() already adds an INNER JOIN on the same table, so the new one is completely unnecessary.

    We can safely remove this extra LEFT JOIN.

  2. Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1 no such function: EXP
    

    SQLite does not natively support most math functions that other DBs do, and the same 'recent' ranking from the point above uses EXP() for its scoring algorithm.

    Fortunately, we can simply register PHP's native exp() function as a custom SQLite function, just like we do for POW().

  3. Drupal\search\SearchQuery does a sql CAST() to decimal, but SQLite does not have a decimal data type. Additionally, any precision suffix of the data type (e.g. (10,4)) is ignored by SQLite, but for compatibility with other DBs, no exception is thrown if it's specified.

    For a decimal precision of 4, as needed by the search ranking system, we can safely use DOUBLE PRECISION which which is supported in MySQL, PostgreSQL and SQLite translates it to FLOAT.

Test run with the attached patch applied:

Drupal test run
---------------

Tests to be run:
  - Drupal\search\Tests\SearchRankingTest

Test run started:
  Sunday, March 29, 2015 - 01:16

Test summary
------------

Drupal\search\Tests\SearchRankingTest                        115 passes                           30 messages

Test run duration: 30 sec

Status: Needs review » Needs work

The last submitted patch, 2: 2454731.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
2.24 KB

I need to read the docs better :) MySQL has a very limited set of types that it can cast to (https://dev.mysql.com/doc/refman/5.5/en/cast-functions.html#function_cast), so let's just switch to ROUND() which works everywhere.

I ran the test on all 3 database engines and they all pass.

jhodgdon’s picture

Thanks for looking into this! This is changing code in the Search module, so it would have been good to have it in the Search component, but it's also changing SQLite, so. Anyway. I appreciate that you added a comment to a Search issue alerting me to this so I could review.

Regarding the Search parts of this patch:

a) Removing the join in node.module in node_search_rankings() -- agreed that is fine. At some point in the past that join was necessary because in the NodeSearch plugin, we were using the node table to join with the search index, but these days we are joining with node_fiedl_data and alias 'n' so the patch is correct there.

b) I do not think that the part in SearchQuery is OK. I have tried using ROUND before and it did not work. I don't recall why... but perhaps it was PostgreSQL? We need to run *all* the search tests in PostgreSQL (not just the SearchRankingTest) and verify this does not break them before I would accept that part of the patch, and we should also make sure it works with settings in the UI. I'm pretty sure I've tried it that way before and it wasn't OK. I just don't remember why... let me see if I can find the issue.

I'm talking about:

+++ b/core/modules/search/src/SearchQuery.php
@@ -509,7 +509,7 @@ public function addScore($score, $arguments = array(), $multiply = FALSE) {
       $i = count($this->multiply);
       // Modify the score expression so it is multiplied by the multiplier,
       // with a divisor to renormalize.
-      $score = "(CAST (:multiply_$i AS DECIMAL(10,4))) * COALESCE(($score), 0) / (CAST (:total_$i AS DECIMAL(10,4)))";
+      $score = "(ROUND(:multiply_$i, 4)) * COALESCE(($score), 0) / (ROUND(:total_$i, 4))";
...
-      $score = implode('((CAST (:normalization_' . $this->relevance_count . ' AS DECIMAL(10,4))) * i.score * t.count)', $pieces);
+      $score = implode('((ROUND(:normalization_' . $this->relevance_count . ', 4)) * i.score * t.count)', $pieces);

c) The code in statistics.module -- same as (b).

So, here are some related issues where we settled on this formulation:
#303574: Search Ranking Recency scoring algorithm
#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken
#19629: Search ranking broken on pgsql
#2195907: Search ranking for number of views fails in PostgreSQL

That last one looks like the main one... So we need to run ****all***** of the search tests on PostgreSQL with this patch before I would say it is OK. I do not think it will work. And if it does I would like to find out why it wasn't working before and what has changed, given the debugging that went on in that last issue.

amateescu’s picture

Thanks for providing some background on the cast to decimal part. I'm attaching test runs on PostgreSQL for all the search group and it seems that this patch does not breaking anything and it's actually fixing a test :) edit: nope, that was just a random fail. I got it again in a different test class after running the search group again without the patch.

I just read those 4 issues you mentioned and I it seems that ROUND() was not considered in any of them.. maybe you tried that only locally and didn't document the attempt in a comment?

As for why this works on PostgreSQL, that's because ROUND() does a CAST() to numeric internally on the first argument, as explained here: http://www.postgresql.org/docs/9.3/static/typeconv-func.html#AEN21484

Also, the comments needed a small update and I also found another place where I didn't change the cast() to round().

Status: Needs review » Needs work

The last submitted patch, 6: 2454731-6.patch, failed testing.

amateescu queued 6: 2454731-6.patch for re-testing.

amateescu’s picture

Status: Needs work » Needs review

The previous test run failed with "Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].", nothing alarming.

jhodgdon’s picture

Status: Needs review » Needs work

Excellent! Thanks for the investigation.

One minor thing: do you think we should add the notes about ROUND in the SearchQuery class where that is used, similar to the comments that are in node.module, comment.module, and statistics.module?

Other than that this is RTBC. Let's add those comments. Don't want this question to come up again. ;)

jhodgdon’s picture

And can I just add I am VERY HAPPY to get rid of those cast() things. ROUND sounds better. Those queries are weird.

bzrudi71’s picture

Yes, nice cleanup! Just cross checked that we have no new fails or exceptions with patch applied on PG - perfect :-)

amateescu’s picture

Status: Needs work » Needs review
FileSize
6.23 KB
1.86 KB

Sure thing, copied the comment to SearchQuery as well.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great! Test bot is happy with MySQL. amattescu is happy with SQLite. bzrudi71 is happy with PG. Comments look good (thanks for catching that one in comment.module). Let's do this!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Actually... Can we just do one more manual test? I have experienced problems with this part of the code and am not absolutely certain it works in practice in the UI. I *think* the tests are good but every once in a while things are different in UI vs. tests and I'd just like to make sure.

So... I think it would be useful if we could do the following, on SQLite, PostgreSQL, and MySQL:
- Turn on Comment and Statistics modules (and Search)
- Make a node with a comment, and another one without a comment
- Go to search settings. In the results weighting section, give every option a positive weight so they are all turned on.
- Run cron to index
- Try a couple of searches.

I'll test MySQL shortly and report back.

jhodgdon’s picture

Just to clarify, to edit the rankings, you go to admin/config/search/pages/manage/node_search [Admin > Configure > Search pages, find the Node plugin and edit there]

And it works fine in MySQL.

bzrudi71’s picture

Works fine with PG! Tested with different influence levels and the result orders change to what I expected.

amateescu’s picture

Same with SQLite. I followed the steps from #15 and then added incrementing influence levels (1 to 5). The last test was to put a higher influence (6) on the first option (Number of comments) and that order of results was updated correctly by putting the article with one comment above the second one with no comments.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for taking that extra step of manual testing! I will sleep better. ;)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed bd1620f and pushed to 8.0.x. Thanks!

  • alexpott committed bd1620f on 8.0.x
    Issue #2454731 by amateescu: SQLite: Fix search\Tests\SearchRankingTest
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.