MultiValueSorter::add() and Enquire::set_sort_by_value() inconsistency

[Olly Betts]

Daniel Menard in comment:ticket:311:15 points out:

Just a note to point out that the logic seems to be the opposite for MultiValueSorter:

!MultiValueSorter::add(Xapian::valueno valno, bool forward=true); Enquire::set_sort_by_key(Xapian::Sorter *sorter, bool reverse=true);

The default direction is good for MultiValueSorter::add(), but the parameter name is annoyingly inconsistent.

Not sure I see a good fix other than renaming the class and/or method.

Marking milestone:1.1.7 for now at least - we should either address this in the 1.1.x series, or decide the best option is not to.

[Olly Betts]

This needs to be resolved before 1.2.0, if only by deciding "WONTFIX" so setting as "highest" priority.

[Olly Betts]

I'm thinking that this is worth fixing - this sort of API inconsistency is likely to trip up users, and at best it is ugly. MultiValueSorter was added during 1.0.x, so we can't just change the default, but it is fairly new and fairly specialised, so it probably doesn't yet have a huge number of users, so I think the pain of migrating it worthwhile in this case.

MultiValueSorter::add(7) is fine, but the two parameter form has the second parameter with the wrong sense. I think that means that we need to rename the class or the method really - my thoughts are either:

• Rename the class to MultiSorter and provide the existing class for compatibility (but deprecate it).

• Rename the method to add_value() and provide the existing add() method for compatibility (but deprecate it).

Document also has an add_value() method, and classes which implement things using values tend to have "Value" in their name, so perhaps the second is the better approach.

Thoughts welcome.

Marking as milestone:1.1.3.

[Olly Betts]

A related thought - aside from the name, Sorter could be used to allow building collapse keys dynamically. MultiValueSorter is perhaps less useful for that purpose - while being able to collapse if all of N values match, there's no useful meaning of the "direction" flag. So perhaps Sorter would be better named more generically (KeyMaker?) Or perhaps we should just have a separate base class for building collapse keys.

[Richard Boulton]

Having the same base class for building keys for sorting or collapsing sounds like a sensible approach to me - if collapsing by binary comparison isn't desirable for some reason, it seems likely that collapsing by items which sort as equal will be.

KeyMaker seems a reasonable name to me. We'd:

• add a KeyMaker base class.
• make Sorter a subclass of this, to allow API compatibility.
• deprecate Sorter and MultiValueSorter.
• add a MultiValueKeyMaker class with the desired semantics for its add_value() method.

[Olly Betts]

Well, collapsing with a pairwise operator would require a major reimplementation of the current approach. In fact, I'm not sure it can be done efficiently if the operator is "these two are equivalent" we need to compare a candidate to all current proto-mset entries. If it's an operator< sort order, we have the same issues as with sorting where the user can crash or hang the sort implementation by violating the well-ordered requirement. It's more consistent to have generated keys for both too.

[Olly Betts]

I've implemented the rename to KeyMaker in trunk r13516.