lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eirik Lygre <eirik.ly...@gmail.com>
Subject Re: Patch for SOLR-3926: A better way to get current sort information from solrj.SolrQuery
Date Mon, 19 Nov 2012 16:09:54 GMT
On Mon, Nov 19, 2012 at 3:23 PM, Eirik Lygre <eirik.lygre@gmail.com> wrote:
>
>
> On Sat, Nov 17, 2012 at 11:41 AM, Eirik Lygre <eirik.lygre@gmail.com>wrote:
>
>>
>>   // Functional, symbolic api. Changes are serialized immediately
>>   Map<String, ORDER> getSort()  // Returns immmutable, ordered map
>>   setSort(LinkedHashMap<S,O>);
>>   addSort(String, ORDER);
>>   removeSort();
>>   removeSort(String);
>>
>> So, a couple of questions out of this:
>> - Do we want to maintain backward compatibility  i.e. the use case with
>> setting the string and then manipulating it?
>> - Is there a clear point for serialization, so that we can serialize "at
>> the end" only?
>>
>
> Two comments for the api:
>
> 1) There is a use case that might be fairly common, in which first a
> number of sorts are set, and then the user wants to change the direction of
> that sort. Presumably this happens over time, and over several user
> interactions, but it would probably look like this:
>
> q.addSort ("a", ASC).addSort("b", ASC).addSort("c", ASC);
> // Now change the sort order of a, while maintaining a as the first field
> in the sort list
> q.setSort("a", DESC); // Don't like; setSort() feels like it should remove
> others
> q.toggleSort("a"); // Works, but I'd prefer an api where I decide the
> direction (e.g. not toggle)
> q.toggleSort("a", DESC) // Works, but the word toggle is not really good
> q.changeSort("a", DESC); // Works, though it is not clear what to do if
> "a" is not already in the sort list
> q.changeOrAddSort("a", DESC); // This is my preferred semantics, but what
> about the method name?
> q.setSort(new LinkedHashMap(q.getSort())
>
> 2) Also, I'm not sure I like the method name "removeSort()" to remove all.
> Either "removeAllSorts()" or "clearSort()", with a preference to the latter
> (it matches "clear()" which is used to clear the entire
> SolrQuery/ModifiableSolrParams)
>
> // Functional, symbolic api. Changes are serialized immediately
> clearSort();
> Map<String, ORDER> getSort()  // Returns immutable, ordered map
> setSort(LinkedHashMap<S,O>);
> addSort(String, ORDER);
> changeOrAddSort (String, ORDER)
> removeSort(String);
>

Attached is a patch; I'll put it up on jira once we're kinda happy with it
:-)

1) I renamed the new method (#1 above) to "addOrUpdateSort()". Probably not
a particularly good parallel, but I found
org.apache.lucene.index.FieldInfos.addOrUpdate(), and reused that convention
2) I removed the new raw string methods getSortString() and
setSortString(). The point of this exercise is to use a symbolic api, and
if you want the raw field, use get()/set() with CommonParams.SORT.

The attached patch now contains a few fixes to the old api, tests for those
fixes, the new api with javadoc, and tests for the new api.

-- 
Eirik

There is no high like a tango high
There is no low like a tango low

Mime
View raw message