Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff.txt | 1.86 KB | amateescu |
#13 | 2454731-13.patch | 6.23 KB | amateescu |
#6 | postgresql-test-run-after.txt | 4.22 KB | amateescu |
#6 | posgresql-test-run-before.txt | 7 KB | amateescu |
#6 | interdiff.txt | 2.62 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedComment #2
amateescu CreditAttribution: amateescu commentedThis test exposes multiple problems in the search module:
This happens because the 'recent' ranking defined in
node_ranking()
adds a LEFT JOIN on thenode_field_data
table without specifying the table alias for the search_indexsid
column. ButDrupal\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.
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 forPOW()
.Drupal\search\SearchQuery
does a sqlCAST()
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:
Comment #4
amateescu CreditAttribution: amateescu commentedI 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.
Comment #5
jhodgdonThanks 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:
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.
Comment #6
amateescu CreditAttribution: amateescu commentedThanks 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 aCAST()
to numeric internally on the first argument, as explained here: http://www.postgresql.org/docs/9.3/static/typeconv-func.html#AEN21484Also, the comments needed a small update and I also found another place where I didn't change the cast() to round().
Comment #9
amateescu CreditAttribution: amateescu commentedThe previous test run failed with "Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].", nothing alarming.
Comment #10
jhodgdonExcellent! 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. ;)
Comment #11
jhodgdonAnd can I just add I am VERY HAPPY to get rid of those cast() things. ROUND sounds better. Those queries are weird.
Comment #12
bzrudi71 CreditAttribution: bzrudi71 commentedYes, nice cleanup! Just cross checked that we have no new fails or exceptions with patch applied on PG - perfect :-)
Comment #13
amateescu CreditAttribution: amateescu commentedSure thing, copied the comment to SearchQuery as well.
Comment #14
jhodgdonGreat! 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!
Comment #15
jhodgdonActually... 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.
Comment #16
jhodgdonJust 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.
Comment #17
bzrudi71 CreditAttribution: bzrudi71 commentedWorks fine with PG! Tested with different influence levels and the result orders change to what I expected.
Comment #18
amateescu CreditAttribution: amateescu commentedSame 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.
Comment #19
jhodgdonThanks for taking that extra step of manual testing! I will sleep better. ;)
Comment #20
alexpottThis 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!