I have added a proposed patch to the jira issue, at https://issues.apache.org/jira/browse/SOLR-3926?focusedCommentId=13502346&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13502346 .
The proposed satisfies my original needs, using a symbolic api as proposed by Yonik. I'm hoping this is now sufficiently well developed to be applied to both the 4x-branch and the trunk moving forward. On Mon, Nov 19, 2012 at 5:09 PM, Eirik Lygre <[email protected]> wrote: > > > > On Mon, Nov 19, 2012 at 3:23 PM, Eirik Lygre <[email protected]>wrote: >> >> >> On Sat, Nov 17, 2012 at 11:41 AM, Eirik Lygre <[email protected]>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 > -- Eirik There is no high like a tango high There is no low like a tango low
