Add MatchSpy class distinct from MatchDecider with support for remote use

[Richard Boulton]

Currently, Enquire::register_match_decider() simply stores the values passed to it in the internals of the Enquire object. These values never get used.

Either register_match_decider() should be removed, or (more probably) the values should be used in the remote match case to allow match deciders registered with the server to be used.

For now, I've added a note in the documentation comment that this method effectively does nothing.

[Olly Betts]

This method anticipates remote backend support for MatchDeciders, so removing it isn't helpful unless we come up with a better approach.

We don't seem to have a bug for lack of remote MatchDecider support, so reusing this one.

The main difficulty I can see is that the MatchDecider-derived object would need to be serialised across before the match and back again afterwards (assuming the subclass has data members). That's going to put extra requirements on user implemented subclasses of MatchDecider, so this definitely can't go into 1.0.x.

[Olly Betts]

Thinking about this more, the approach of register_match_decider seems wrong - it would be better (for consistency if nothing else) to handle this like we do for Weight subclasses - add a method to the class which returns the name. We'll also need to allow the object to serialise and unserialise any state.

So I think deprecating the current Enquire::register_match_decider() would be best - I'm just building and will commit that change shortly. Since it doesn't actually do anything, it seems unlikely anyone is using it, so perhaps it could be dropped in 1.1.0 - for now I've marked it for removal in 1.2.0.

[Richard Boulton]

Just to note - I'm going to be working on this over the next couple of weeks, to enable match spies and match deciders to be used with remote databases.

[Olly Betts]

Just to update the info already here, register_match_decider has now been removed.

Richard and I have just been discussing on IRC, and we're currently thinking that we'll deprecate MatchDecider in favour of the more flexible PostingSource, and add a new MatchSpy class which can "look but not touch" - i.e. operator() returns void.

So I guess this is now "add MatchSpy with support for remote use".

[Richard Boulton]

I've been working on a patch to do this. The hardest part so far has been adding a MatchSpy parameter to get_mset(). Since we're planning to deprecate MatchDecider, I would like to add a new get_mset() overload with the signature:

    MSet get_mset(Xapian::doccount first,
                  Xapian::doccount maxitems,
                  Xapian::doccount checkatleast,
                  const RSet * omrset,
                  MatchSpy * matchspy) const;

However, I can't make this work without creating ambiguities with the existing get_mset() methods. My favoured solution for now is to add this method with a different name (ie, get_mset_with_matchspy), but this seems less than ideal.

Other than this difficulty, I'm making good progress - I'll probably apply the patch to the matchspy branch, and modify the matchdecider subclasses there to be matchspy subclasses, since this will offer an easy way to test with the remote backend.

[Olly Betts]

The ambiguities will be for the case where NULL is passed for that parameter. There must be a better solution than adding a parallel method name though.

I assume you're planning to merge this with the matchspy branch now, so this is no longer a 1.1.2 thing so moving it to 1.1.4 for now.

[Richard Boulton]

Yes, planning to merge this with the matchspy branch now.

I spent about an hour trying to come up with a solution for the overloaded get_mset() thing; the best I could come up with was adding the MatchSpy option as a pass-by-reference signature with no default - this way it's never ambiguous. ie, add:

MSet get_mset(Xapian::doccount first,
              Xapian::doccount maxitems,
              Xapian::doccount checkatleast,
              const RSet * omrset,
              MatchSpy & matchspy) const;

I'm not sure whether this is bad, really; passing by reference seems cleaner, somehow. However, using pointer allows the caller to build up things like the matchspy parameter, and use the default value of NULL if no matchspy is wanted. This signature is likely to lead to lots of code like:

if (matchspy_ptr != NULL) {
    mset = enq.get_mset(0, 10, 0, NULL, *matchspy);
} else {
    mset = enq.get_mset(0, 10, 0, NULL);
}

which seems a shame.

[Olly Betts]

Would be good to sort out, as we can't really deprecate the current matchspy until we add the new one. Not a blocker though.

[Olly Betts]

This was addressed the merge from the postingsources branch in r13243.