The current code for drupal 5.18 (encountered the bug on) and 6.12 adds a ranking clause like so by default:
%d * POW(2, (GREATEST(n.created, n.changed, c.last_comment_timestamp) - %d) * 6.43e-8)
This is then used to calculate the final relevance of all nodes that were brought up in the search results. However, if there is no entry in node_comment_statistics for a given node, c.last_comment_timestamp will = NULL. In both the versions of mysql I tested with GREATEST(any number, NULL) will return NULL; subtraction with NULL returns NULL; multiplication with NULL returns NULL. Finally, the relevance is returned as NULL.
The attached patch is against drupal 5. Drupal 6 has MAX(c.last_comment_timestamp), and MAX(NULL) is also NULL. So, the patch to drupal 6 could simply be MAX(c.last_comment_timestamp) || 1 (or some such low number). Something simily might need to be applied in the 'status' switch in node_search in drupal 5, but I'll leave that up to someone who knows the code better.
Comment | File | Size | Author |
---|---|---|---|
#19 | 488166-fix-reroll.patch | 1.26 KB | jhodgdon |
#18 | 488166-fix-search-relevance-d6.19.patch | 1.07 KB | EmanueleQuinto |
#7 | 488166-fix-search-relevance-d6.16.patch | 992 bytes | EmanueleQuinto |
#5 | 488166-fix-search-relevance-d6.patch | 1.6 KB | brianV |
#2 | search_relevance_fix_drupal6.txt | 729 bytes | jhutton |
Comments
Comment #1
brianV CreditAttribution: brianV commented+1 subscribe.
Should be fixed in 5.x and 6.x. This is not an issue in D7 as last_comment_timestamp isn't used.
Comment #2
jhutton CreditAttribution: jhutton commentedBumped to 6.12 since the bug also appears to be present there. It looks as if Drupal 7 does not contain the bug, however, since it does
'score' => 'POW(2.0, (GREATEST(n.created, n.changed) - %d) * 6.43e-8)'
leaving out last_comment_timestamp entirely.
Attached is a patch against drupal 6.12.
Comment #3
brianV CreditAttribution: brianV commentedLooks good.
Bumping back to 6,x-dev since that is where it would be fixed.
Comment #4
Gábor HojtsyI'd like to request a line of comment above this code. I see that since 1 will be lower then the other MAX-es, you get some real value, but that would be good to document. Otherwise it could easily get people puzzled on what the hell happens there.
Comment #5
brianV CreditAttribution: brianV commentedUpdated, with a comment explaining things. The comment is sort of long, but as short as I could make it while adequately explaining the issue.
Comment #6
petercasier CreditAttribution: petercasier commentedProblem is still there in 6.16
patch should now be:
$ranking[] = '%d * POW(2, (GREATEST(MAX(n.created), MAX(n.changed), MAX(c.last_comment_timestamp) || 1) - %d) * 6.43e-8)';
Comment #7
EmanueleQuinto CreditAttribution: EmanueleQuinto commentedHere attached patch for 6.16
Comment #8
petercasier CreditAttribution: petercasier commented.. and still there in 6.17 too
Comment #9
EmanueleQuinto CreditAttribution: EmanueleQuinto commented... the patch works for 6.17 and 6.x-dev too (as for
node.module,v 1.947.2.24 2010/06/02 12:11:07 goba
), it only seems to need review.Comment #10
Jim Kirkpatrick CreditAttribution: Jim Kirkpatrick commentedPatch applies, search now works properly - thanks!
Marking as tested.
Comment #11
Gábor HojtsyGreat, thanks for the testing. Committed.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorry to reopen, but this is very broken:
|| 1
doesn't really make any sense :pThe search module is now broken on PostgreSQL. Yay, fun.
What you wanted here is:
GREATEST(MAX(n.created), MAX(n.changed), COALESCE(MAX(c.last_comment_timestamp), 0))
.It would also probably makes more sense to move the MAX() out of the GREATEST(), but the result would be unaffected and it's another issue anyway.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedMarked #488166: Search relevance calculation fails if last_comment_timestamp is NULL as a duplicate.
Comment #14
jhodgdonDamien meant to say that #882194: PostgreSQL query error when searching was marked as a duplicate, I think.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks Jennifer, that somewhat makes more sense.
Comment #16
jpulles CreditAttribution: jpulles commentedIt seems solved by removing the || 1 part. A 'coalesce' function is not needed because the timestamp is NOT NULL; just the greatest value of 3 timestamps is needed.
GREATEST(MAX(n.created), MAX(n.changed), MAX(c.last_comment_timestamp))
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commented@jpulles: have you seen the title of the issue? :)
Comment #18
EmanueleQuinto CreditAttribution: EmanueleQuinto commentedThis patch should be at least SQL-92 conformant. It uses COALESCE now.
I don't have a PostgreSQL environment, apologize for this, then should be tested there...
Comment #19
jhodgdonJust as a note: When supplying patches for Drupal, it's best to (if possible) roll them using cvs diff, and at a minimum, roll them from the Drupal root, rather than at the level of the particular file you are patching.
That said, I have set up a PostgreSQL environment, reproduced the SQL error that is happening now, then applied the patch above. The errors went away, and my searching tests (on contrib module Porter Stemmer) now pass.
So here is the same patch, rerolled from Drupal head.
Comment #20
jpulles CreditAttribution: jpulles commented@Damien: Yes, I read the title, but thought that the last_comment_timestamp could never be null since it is defined as 'NOT NULL' in the database schema.
Comment #21
jhodgdonMany people are reporting this -- WE NEED TO FIX IT SOON!!!!
#888164: After update search has ceased to work
http://drupal.org/node/885026
etc.
Comment #22
lifepillar CreditAttribution: lifepillar commentedSubscribe.
Comment #23
jhodgdonAnyone having this problem using Postgres and Drupal 6.19 can apply the patch in #19 and likely fix the problem (if someone besides me could verify, that might help).
If you are not familiar with how to apply patches, take a look at the attached patch file. Basically the lines starting with - need to be removed and replaced with the lines starting with +. You can do this by hand in a text editor, since this patch is so small. Be sure to remove the + sign at the beginning of the two lines you are adding!!
Comment #24
kecsi CreditAttribution: kecsi commentedYes. I had same issue here using 6.19, Postgres. Fixed manually. Thanks.
Comment #25
mcm.drupal CreditAttribution: mcm.drupal commentedI'm not familiar with the testing process here... and am confused by:
I don't know how to contribute my two cents worth, which is that the patch fixed my problem on 6.19 with PG 8.3.7.
Comment #26
gionn CreditAttribution: gionn commented#19 Works for me. (Debian Lenny apache2+postgres)
Comment #27
andrea.vivaldi CreditAttribution: andrea.vivaldi commentedsubscribe
Comment #28
gmh04 CreditAttribution: gmh04 commented#19 works for me postgres 8.4.4
Comment #29
PMunn CreditAttribution: PMunn commentedWorks for me on PostgreSQL 8.4.4! Woohoo! Thank you jhodgdon for making this patch!
Comment #30
bendiy CreditAttribution: bendiy commentedOMG! patching now...
Comment #31
jorbot CreditAttribution: jorbot commentedPatch worked for me. THANK YOU!
Comment #32
christoph CreditAttribution: christoph commentedFor those who don't want to patch their production site I found the following worked as a temporary measure.
Go to Admin>Settings>Search and set the Factor 'Recently Posted' to 0. (Will have to remember to set back again on the next release).
Comment #34
marlatt CreditAttribution: marlatt commentedThanks christoph for this note, as no one has solved this problem yet and some of us with postgres don't want to be in the wild with core security vulnerabilities.
Comment #35
jhodgdonD6 patches are failing with the same error. There's no problem with this patch.
Comment #36
marlatt CreditAttribution: marlatt commentedI wasn't aware of that, thank you.
Comment #38
jhodgdonre-testing is not going to help.
#961172: All D6 Core patches are failing
Comment #39
diaxpro CreditAttribution: diaxpro commentedThis is my error:
user warning: Operand should contain 1 column(s) query: SELECT COUNT(*) FROM (SELECT i.type, i.sid, 5 * (3,6765835972501 * SUM(i.score * t.count)) + 5 * POW(2, (GREATEST(MAX(n.created), MAX(n.changed), COALESCE(MAX(c.last_comment_timestamp), 0)) - 1288716585) * 6.43e-8) + 5 * (2.0 - 2.0 / (1.0 + MAX(c.comment_count) * 0,14285714285714)) + 5 * (2.0 - 2.0 / (1.0 + MAX(nc.totalcount) * 0,0050251256281407)) 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 INNER JOIN active_translation ON n.nid = active_translation.es LEFT JOIN node_comment_statistics c ON c.nid = i.sid LEFT JOIN node_counter nc ON nc.nid = i.sid WHERE n.status = 1 AND (i.word = 'parroquia') AND i.type = 'node' GROUP BY i.type, i.sid HAVING COUNT(*) >= 1) n1 in ../search/search.module on line 958.
My partner show that in sql change the "," for "." work i find the solution...
Comment #40
jhodgdondiaxpro: is this the same problem as has been reported above, and does the patch in #19 above fix it?
If it is a separate problem, please file a new issue. Thanks.
Comment #41
diaxpro CreditAttribution: diaxpro commentedI'm sorry but #19 don't work for me and the solution that i find is in http://drupal.org/node/389452 on coment #2
Thx.
Comment #42
jhodgdonYour problem is different from this error report. Your problem is that SQL doesn't support commas as decimal points, but for some reason, your system is creating queries with commas as decimal points. The solution you found is telling PHP not to do that. So it's unrelated to this issue.
Comment #43
quiptime CreditAttribution: quiptime commentedPatch #19 works on Ubuntu 8, PG 8.3.12 and D 6.19.
I'm very surprised. Many users report that the patch #19 works.
Why is not taken over the patch?
Comment #44
vevhlos CreditAttribution: vevhlos commentedThanks @#32! brilliant thought!
Comment #45
jhodgdon#19: 488166-fix-reroll.patch queued for re-testing.
Comment #47
jhodgdonI had thought that #961172: All D6 Core patches are failing was fixed, and so I asked for a retest of this patch, but it's not fixed.
Comment #48
dsarch CreditAttribution: dsarch commentedHi,
Please release a new version with this patch (#45) as it totally breaks things in postgresql here. I did exactly this fix (coalesce) and it works here.
Is it possible to generate another release at least with this correction ? This is a critical bug and it is fixed, so there is not holding it.
Thanks for the work.
Comment #49
morthylla CreditAttribution: morthylla commented#32 Thank you! A simple and elegant solution. I only have to remember to modify it later ^_^
Comment #50
atavio CreditAttribution: atavio commented#19 & #32 worked for me.
Drupal 6.19
Postgres 8.4
Thanks! ;)
Comment #51
Gábor HojtsyThanks, committed.
Comment #53
pvpaneque CreditAttribution: pvpaneque commentedthanks my friend for that configuration , i was looking for the solution of this problem a long time ago and this setting works perfectly , thanks so much