A fix for the joining threads bug with Ferret

[Gregor Hoffleit-2]

I did a little bit research regarding the problem that joining threads
isn't persistent (as described in [1]-[3]).

I managed to track down the problem until the following line in
FerretIndex#sync_message in lib/sup/ferret_index.rb.

  d = { ...  :refs => (entry[:refs] || (m.refs + m.replytos).uniq.join(" ")) }

I have problems to understand what this line is supposed to do.

For me, it always evaluates to "entry[:refs]" (even if that's an empty
string!), losing the reference in the modified message m, which was added
by add_ref. Therefore the manual join is always lost.

With my limited Ruby knowledge, my quick and dirty fix was:

    if entry[:refs]!="" then
       d[:refs]=entry[:refs]
    else
       d[:refs]=(m.refs + m.replytos).uniq.join(" ")
    end

Is this what the above code is about?


Btw, the code in xapian_index.rb looks much different. Still, I'd like
to see this fixed for Xapian.


Regards,
    Gregor Hoffleit


[1] http://sup.rubyforge.org/ditz/issue-4e501973cea5bd1f28739ae4cea98edce8249895.html
[2] http://rubyforge.org/pipermail/sup-talk/2009-April/002050.html
[3] http://rubyforge.org/pipermail/sup-talk/2009-April/002060.html
_______________________________________________
sup-talk mailing list
sup-talk@...
http://rubyforge.org/mailman/listinfo/sup-talk

[Tero Tilus-2]

Excerpts from Gregor Hoffleit's message:
> With my limited Ruby knowledge, my quick and dirty fix was:
>
>     if entry[:refs]!="" then
>        d[:refs]=entry[:refs]
>     else
>        d[:refs]=(m.refs + m.replytos).uniq.join(" ")
>     end
>
> Is this what the above code is about?

I would expect it to be something alike.  Rails has Object#empty?
which is true (I think) for nil, false, empty list and empty string.
I'd go with

  entry[:refs].to_s != ""

to handle nil as "no refs" too.

--
Tero Tilus ## 050 3635 235 ## http://tero.tilus.net/
_______________________________________________
sup-talk mailing list
sup-talk@...
http://rubyforge.org/mailman/listinfo/sup-talk

[William Morgan-4]

Reformatted excerpts from Gregor Hoffleit's message of 2009-10-23:
> I managed to track down the problem until the following line in
> FerretIndex#sync_message in lib/sup/ferret_index.rb.
>
>   d = { ...  :refs => (entry[:refs] || (m.refs + m.replytos).uniq.join(" ")) }

Great catch. Thank you!

> For me, it always evaluates to "entry[:refs]" (even if that's an empty
> string!), losing the reference in the modified message m, which was
> added by add_ref. Therefore the manual join is always lost.

Yep, that's the problem. In general all those fields are ||= in order to
save the work of computing them again when we're updating a message
(because the way Ferret works is we have to rewrite the entire record
every time we change the labels, so we want that to be as fast as
possible). But for certain fields, including refs, we should regenerate
them because that data can change.

> Btw, the code in xapian_index.rb looks much different. Still, I'd like
> to see this fixed for Xapian.

I believe that the fix for both is simply to set the entry[:refs] every
time, and not bother with ||=. (In fact I think the empty string check
won't work for threads that already have refs set, i.e. joining threads
of size >1.)

I've put this fix on branch thread-joining-fix and merged into next.
Check it out.
--
William <wmorgan-sup@...>
_______________________________________________
sup-talk mailing list
sup-talk@...
http://rubyforge.org/mailman/listinfo/sup-talk