range queries use lexical comparison, not numerical

[sascha_silbe]

While adding a range query example to the test suite I discovered a bug: Range queries currently do lexical comparisons, not a numerical ones. This means that a range query of 9000..11000 will _not_ match 10000.
For the current code this is a minor issue as only the 'timestamp' property supports range queries and all dates between about mid-2001 and 2033 have exactly ten digits (so for regular Journal entries it shouldn't make any difference).

[sascha_silbe]

use xapian.sortable_serialise() for storing/searching numeric values

[tomeu]

>>> xapian.sortable_serialise('2.0')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: in method 'sortable_serialise', argument 1 of type 'double'

I think we should be able to accept strings in propertiestimestamp?.

[sascha_silbe]

Replying to tomeu:

I think we should be able to accept strings in propertiestimestamp?.

For that to work we need to add type information to _QUERY_VALUE_MAP (or special-case timestamp somewhere else in the code) so we know we need to convert the string to an integer first.

I suggest moving this ticket to 0.88.

[tomeu]

Replying to sascha_silbe:

Replying to tomeu:

I think we should be able to accept strings in propertiestimestamp?.

For that to work we need to add type information to _QUERY_VALUE_MAP (or special-case timestamp somewhere else in the code) so we know we need to convert the string to an integer first.

Why not just doing this?

document.add_value(_VALUE_TIMESTAMP, xapian.sortable_serialise(float(properties['timestamp'])))

It will fail if people put there non-numeric values, but at least it won't fail if people happen to pass a dbus string instead of a dbus numeric type.

[sascha_silbe]

Replying to tomeu:

Why not just doing this?

document.add_value(_VALUE_TIMESTAMP, xapian.sortable_serialise(float(properties['timestamp'])))

Because that's only half of the solution and IMO the less important half. Timestamps are usually generated by the sugar.* framework while saving the entry, I can't think of any reason a (Python) activity would want to do it on their own and then I'd rather have it fail right away if they don't pass a numeric type, especially if passing a string for querying doesn't work.
The other half is querying and this is where is makes an important difference: We need to know we're expecting a numeric type in order to invoke sortable_serialise() before comparison. sortable_serialise() produces binary data, so comparing with a decimal string doesn't make any sense.

[tomeu]

Replying to sascha_silbe:

Replying to tomeu:

Why not just doing this?

document.add_value(_VALUE_TIMESTAMP, xapian.sortable_serialise(float(properties['timestamp'])))

Because that's only half of the solution and IMO the less important half.

Well, for me it's important that we don't add more points of failure to activity development.

Timestamps are usually generated by the sugar.* framework while saving the entry, I can't think of any reason a (Python) activity would want to do it on their own and then I'd rather have it fail right away if they don't pass a numeric type, especially if passing a string for querying doesn't work.

I really don't see why you don't think it's good to play a bit safer.

The other half is querying and this is where is makes an important difference: We need to know we're expecting a numeric type in order to invoke sortable_serialise() before comparison. sortable_serialise() produces binary data, so comparing with a decimal string doesn't make any sense.

I'm just saying to accept the timestamp property in both string and numerical dbus types. It of course needs to contain a number, just don't see what we gain by enforcing it to be a numerical dbus type there.

[sascha_silbe]

Replying to tomeu:

I really don't see why you don't think it's good to play a bit safer.

I'm mostly arguing against "playing safe" early on (create() / update()) and failing silently later (find()).

[tomeu]

Replying to sascha_silbe:

Replying to tomeu:

I really don't see why you don't think it's good to play a bit safer.

I'm mostly arguing against "playing safe" early on (create() / update()) and failing silently later (find()).

Sorry, I'm still not getting it. Why would we fail silently when querying?

[sascha_silbe]

Replying to tomeu:

Sorry, I'm still not getting it. Why would we fail silently when querying?

Because timestamp is stored using sortable_serialise() on create()/update(), but if you pass a (decimal) string for timestamp to find() it will use it as-is. Comparing a decimal string to one returned by sortable_serialise() doesn't give any useful result.

[tomeu]

Replying to sascha_silbe:

Replying to tomeu:

Sorry, I'm still not getting it. Why would we fail silently when querying?

Because timestamp is stored using sortable_serialise() on create()/update(), but if you pass a (decimal) string for timestamp to find() it will use it as-is. Comparing a decimal string to one returned by sortable_serialise() doesn't give any useful result.

But if we know that timestamp is a serialized decimal number, why not treat it as such in find()?

[sascha_silbe]

Replying to tomeu:

But if we know that timestamp is a serialized decimal number, why not treat it as such in find()?

That's exactly what I said above:

For that to work we need to add type information to _QUERY_VALUE_MAP (or special-case timestamp somewhere else in the code) so we know we need to convert the string to an integer first.

We can special-case timestamp in find(), but since the impact for 0.86 is minimal I'd rather modify the generic code for 0.88 so it works for other properties (that didn't make it into 0.86 but hopefully will for 0.88) as well.

[sascha_silbe]

fix range query for timestamp to do numerical comparison instead of lexical

[sascha_silbe]

New patch, based on #1437.

[alsroot]

​http://git.sugarlabs.org/projects/sugar-datastore/repos/mainline/commits/0e682f97bf60b57d6991d52be8c0b77c5b8b5b28

[dnarvaez]

Milestone 0.88 deleted