Check that NumberValueRangeProcessor::apply() is usable from bindings

[daniel andersson]

This has been addressed for PHP and Python now - other languages need a testcase adding and any problems it uncovers fixing.

---

Original report:

The following returns an error, even though the right arguments have been passed:

$vrp = new XapianNumberValueRangeProcessor(0, "\$", true);
$vrp->apply((string)"240", (string)"500");

The error returned is:

Fatal error: Type error in argument 2 of NumberValueRangeProcessor_apply. Expected 
SWIGTYPE_p_std__string in xapian.php on line 1217

line 1217 being:

return NumberValueRangeProcessor_apply($this->_cPtr,$begin,$end);

Have tried changing that line to say (string)$begin and (string)$end, same result.

[Olly Betts]

SWIG isn't handling passing the string parameter by non-const reference.

I think this will need custom wrapping.

This may not work for some (or all) of the other bindings either...

[Olly Betts]

We should try to fix this for 1.0.3 if possible.

[Olly Betts]

Fixed (for PHP at least) by patching SWIG and using the latest SWIG version to generate the bindings.

This is the regression testcase I added, which now passes:

$vrp = new XapianNumberValueRangeProcessor(0, '$', true);
$a = '$10';
$b = '20';
$vrp->apply($a, $b);
if (Xapian::sortable_unserialise($a) != 10) {
    print Xapian::sortable_unserialise($a)." != 10\n";
    exit(1);
}
if (Xapian::sortable_unserialise($b) != 20) {
    print Xapian::sortable_unserialise($b)." != 20\n";
    exit(1);
}

Leaving this bug open until I check if this works for all the other languages...

[Olly Betts]

Patch to smoketest.py to show this bug for Python

[Olly Betts]

Richard - this bug manifests for Python too. It looked to me like SWIG had suitable typemaps, but perhaps you need to %apply them or something. The fix for PHP was to add "in" and "argout" typemaps for string&.

I'll check the other languages tomorrow.

[Richard Boulton]

Hmm. Unless I'm missing something, it's not possible for the python test you've written to work. In python, strings are immutable, so we can't change the value of the parameters "a" and "b" from inside the "vrp(a,b)" call, however we implement it.

We could make vrp() return the new values of a and b. Alternatively, we could just leave this as it is, since I'm not convinced it's actually useful to call a vrp() from Python.

[Olly Betts]

We shouldn't just leave it - if something doesn't work for a language, providing a wrapper for it is just a trap for API users.

But I think this has uses - if you wanted to write a VRP subclass in Python which was based on an existing VRP subclass then it's useful to be able to call operator() on the existing class.

In fact, if you want to write a VRP subclass in Python, you're probably going to want to be able to modify the strings passed in anyway...

[Richard Boulton]

Committed a fix for python (__call__() now returns a 3-tuple, containing the slot num, the returned begin and the returned end).

[Olly Betts]

Postponing what's left of this to 1.0.4 in the interests of actually making a release.

[Olly Betts]

Since the use-case for this is to allow subclassing your own VRP in the language, and SWIG/PHP doesn't support such subclassing at present, I'm going to bump this to 1.1.0.

[Olly Betts]

And now to 1.1.1

[Olly Betts]

Triaging milestone:1.1.1 bugs.

[Olly Betts]

Until PHP directors are working, this isn't an important change, and this can be implemented in 1.2.x without causing incompatibilities since this method isn't useful in PHP currently, so bumping.

[Olly Betts]

It would be nicer if the range was returned as a Query object, as that could do neat things like optimising to use filter terms to narrow the range. With that change, the (in)ability to pass strings by reference and have modified values returned wouldn't be an issue.

The issues I see with such a change are:

• migration - I think we could provide a RangeProcessor that works the new way (since it isn't inherently linked to values), and deprecate ValueRangeProcessor, handling it via a RangeProcessor subclass which adapts the result.

• QueryParser wants to know which slot the range is for, so it knows about combining ranges on the same slot with OP_OR and then combine these groups with OP_AND. That isn't unsurmountable, but would probably need us to return both a Query object and some sort of string or number "key" to identify the type of range. Or perhaps this is an overly fussy little detail, which most people wouldn't expect us to worry about. FieldProcessor just returns a Query object after all.

[Olly Betts]

Ah, that's #663.

[Olly Betts]

(In #663) This would address #193 too.

One issue I noticed: QueryParser wants to know which slot the range is for, so it knows about combining ranges on the same slot with OP_OR and then combine these groups with OP_AND. That isn't unsurmountable, but would probably need us to return both a Query object and some sort of string or number "key" to identify the type of range. Or perhaps this is an overly fussy little detail, which most people wouldn't expect us to worry about. FieldProcessor just returns a Query object after all.

[Olly Betts]

The initial reported issue was address long ago, and what remains isn't really relevant once we solve #663, so I don't think there's any benefit in keeping this ticket open now. The last pieces seem to have gone into 1.0.4, but there's no milestone for that, so marking as fixed in 1.0.7.