Serialization for Xapian::Query doesn't work for PostingSource queries.

[Armin Ronacher]

Attempting to serialise a query containing a PostingSource subquery raises a UnimplementedError exception. Much of the code to implement this will be part of the fix to ticket #295 - we will need to provide an extra argument to the Query::unserialise method() to provide a way to lookup possible PostingSource classes by name.

[Olly Betts]

Only requires API additions, not changes, so suitable for 1.0.x.

[Richard Boulton]

(In #120) Remove the unfixed dependencies so we can close this bug - they're all marked for the 1.1.0 milestone.

[Olly Betts]

Bumping milestone to 1.1.1 as there's no patch yet and this isn't an incompatible change.

[Richard Boulton]

Patch implementing this.

[Richard Boulton]

Tests of this feature, in python

[Richard Boulton]

Added patches implementing this, and testing it (just with the python bindings, though).

Current patch won't support serialising queries which use PostingSources, but I think that's a reasonable restriction for now. To support it, we'll need to pass an extra argument to the unserialise method giving the list of PostingSources available to be unserialised to, and add ways to serialise and clone Posting sources. This is implemented in the patch attached to ticket #295, so I'm adding a dependency on that ticket for this bug. I'm planning to implement some C++ level tests for this, and then apply the patch, since I could do with the feature for some upcoming work.

[Richard Boulton]

Changeset [11829] applies this patch, together with some tests, and fixes most of this bug. It's not possible to serialise queries containing postingsources, so I'm updating the title and description to reflect this. Also marking this for 1.1.0, since it's just about done, and the final fix may require a tweak to the API.

[Richard Boulton]

Patch to allow serialisation of queries containing posting sources

[Richard Boulton]

I've just attached a patch which adds a SerialisationContext object which allows users to register their own posting sources (or weighting schemes). The patch modifies the query unserialisation methods to use this, and also modifies the remoteserver to use this internally.

The patch is lacking tests (both in core and in the bindings) - though it passes the existing testsuite. It would probably be best if RemoteServer was also modified to expose the SerialisationContext, too.

[It also fails to delete old registered posting sources or weighting schemes when they're replaced by one of the same name - this is just an oversight which is marked by FIXME in the patch, and which is easily resolved.]

Comments on the design welcome!

[Olly Betts]

It seems unreasonable to tell people not to use serialising for persistence, since that seems an obvious use for this feature, especially for queries. We should at least be able to promise that it'll work when the remote protocol version hasn't changed.

[Richard Boulton]

I've reworded the comment about persistence to say this: [11846].

[Richard Boulton]

Notes about comments from Olly on IRC about the unapplied serialisationcontext.patch (paraphrased by me):

• don't say "the copy is shallow" - talk in terms of reference counted handles? Do whatever is done elsewhere.
• "if when" in a doc comment
• comment about why dtor is needed should just be an internal comment, not documentation.
• @private not needed in private: section
• say "standard ones" instead of "hard-coded ones".
• add comment to remoteserver.cc to show where to register your own subclasses
• update comment in xapian-tcpsrv.cc

[Richard Boulton]

Updated patch implementing SerialisationContext.

[Richard Boulton]

serialisationcontext2.patch contains an updated implementation, addressing all the points mentioned above. It still lacks tests, but other than this, I think it's probably ready to go.

[Olly Betts]

Let's not add yet another blocker to getting 1.1.0 out. We're at the point where a 1.1.0 release is within sight and there are many new features which are solid and a number of people are interested in using. 1.1.0 will be clearly identified as a development release so we shouldn't keep pushing it back by striving for perfection in the 1.1.0 release.

Since this is an entirely new API feature, so we can mark it as experimental if you're worried that the API may need to change.

[Richard Boulton]

Now that I have a nearly complete patch, I'm not so worried about the API needing to change incompatibly: the changes the current patch makes to the files in include/xapian/ are:

• Changing the prototype of a method in Query::Internal (but that's not part of the API, so should be fine, I think). Might break ABI, though; I'd need to do a deeper analysis to determine that.
• Adding static Query unserialise(const std::string & s, const SerialisationContext & ctx); to the Query class. This shouldn't be any kind of problem.
• Adding "friend class SerialisationContext" to Xapian::Weight. This shouldn't be any kind of problem.
• Adding xapian/serialisationcontext.h - also shouldn't be any problem.

If I get a completed patch, I'd still like to consider applying it before 1.1.0, (unless we're really getting close to having 1.1.0 ready; currently I think we're still around a month of work from it) - my experience is that patches sitting in the tracker tend to decay quite quickly, and need a fair bit of work to apply. There is quite a bit of code here, but it doesn't have many deep interactions with other parts of xapian, so I feel that there's a fairly low risk of destabilisation due to it.

[Olly Betts]

ABI changes aren't an issue for 1.1.x (though ideally we should probably try to group ABI breaking changes into just a few point releases).

I was really just trying to set the milestone to reflect the actual status - this doesn't deserve to block a 1.1.0 release, so milestone:1.1.0 is the wrong setting.

I don't currently feel I have a good grasp of what really needs to be done before 1.1.0, hence I've been checking through milestone:1.1.0 bugs.

Other non-destabilising changes are generally OK too, though if we're working on those that presumably means we aren't working on the changes that are required to release, so some self-discipline is probably going to be needed here!

[Richard Boulton]

Patch with tests, ready to apply I think

[Richard Boulton]

I've attached a patch which contains unittests, and is fully ready to apply, I think.

I'll wait a couple of days for comment, and then apply it if there are no problems.

[Richard Boulton]

Applied this patch, with a few further tweaks, so marking this ticket as closed.

Also marking as for the 1.1.0 milestone, since it will be in that release.