Return-Path: X-Original-To: apmail-lucene-dev-archive@www.apache.org Delivered-To: apmail-lucene-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id E5763D814 for ; Wed, 21 Nov 2012 22:01:20 +0000 (UTC) Received: (qmail 4187 invoked by uid 500); 21 Nov 2012 22:01:19 -0000 Delivered-To: apmail-lucene-dev-archive@lucene.apache.org Received: (qmail 4094 invoked by uid 500); 21 Nov 2012 22:01:19 -0000 Mailing-List: contact dev-help@lucene.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@lucene.apache.org Delivered-To: mailing list dev@lucene.apache.org Received: (qmail 4086 invoked by uid 99); 21 Nov 2012 22:01:19 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 21 Nov 2012 22:01:19 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of eirik.lygre@gmail.com designates 209.85.212.48 as permitted sender) Received: from [209.85.212.48] (HELO mail-vb0-f48.google.com) (209.85.212.48) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 21 Nov 2012 22:01:13 +0000 Received: by mail-vb0-f48.google.com with SMTP id l22so6884843vbn.35 for ; Wed, 21 Nov 2012 14:00:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=yo01Z5JKv+CK6UKaGP+0yaUFYH7EJ555fi8LY5MxNpY=; b=t5J7y8bsD8bAxS77DmK1oD2g1FwF4jT9vsUG54L1CGpQWIZNRi0SHIlMkXrvVZ5MbF OLEm4Th5Y8lw7xw80YX8HNfponr94ho4cEfBgSjjdEhyicVkPHYr/hiwrksenp+tYUJz 3HZBW9xoa0r9m9ebhUsGBXSnsKI6Z6lhh94ojAczHK+0qCBwftjyBUBAGl02j7mzJqdg 4P6kwrh4HL6pIYCHjLL9ezqrd5AUJC50z2fIff42Qhg0i6OtQLvVv5rBVUSI2hBNUwEu Gqs01tX2jG1ZVmmzoD22BDLDw1O+ipZQi3+3m4jIiSS0rcsu0QQT4/TcOrlMmkPR3MJu tt+g== MIME-Version: 1.0 Received: by 10.52.68.207 with SMTP id y15mr774526vdt.65.1353535252170; Wed, 21 Nov 2012 14:00:52 -0800 (PST) Received: by 10.58.255.136 with HTTP; Wed, 21 Nov 2012 14:00:52 -0800 (PST) In-Reply-To: References: Date: Wed, 21 Nov 2012 23:00:52 +0100 Message-ID: Subject: Re: Patch for SOLR-3926: A better way to get current sort information from solrj.SolrQuery From: Eirik Lygre To: dev@lucene.apache.org Content-Type: multipart/alternative; boundary=20cf3079b62ae589a004cf087b1b X-Virus-Checked: Checked by ClamAV on apache.org --20cf3079b62ae589a004cf087b1b Content-Type: text/plain; charset=ISO-8859-1 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 wrote: > > > > On Mon, Nov 19, 2012 at 3:23 PM, Eirik Lygre wrote: >> >> >> On Sat, Nov 17, 2012 at 11:41 AM, Eirik Lygre wrote: >> >>> >>> // Functional, symbolic api. Changes are serialized immediately >>> Map getSort() // Returns immmutable, ordered map >>> setSort(LinkedHashMap); >>> 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 getSort() // Returns immutable, ordered map >> setSort(LinkedHashMap); >> 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 --20cf3079b62ae589a004cf087b1b Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable I have added a proposed patch to the jira issue, at=A0https://issues.apache.org/jira/browse/SOLR-3926?focusedCommentId= =3D13502346&page=3Dcom.atlassian.jira.plugin.system.issuetabpanels:comm= ent-tabpanel#comment-13502346.

The proposed satisfies my original needs, using a symbolic a= pi as proposed by Yonik. I'm hoping this is now sufficiently well devel= oped to be applied to both the 4x-branch and the trunk moving forward.


On Mon, Nov 1= 9, 2012 at 5:09 PM, Eirik Lygre <eirik.lygre@gmail.com> = wrote:



On Mon, Nov 19, 2012 at 3:23 P= M, Eirik Lygre <eirik.lygre@gmail.com> wrote:

On Sat, Nov = 17, 2012 at 11:41 AM, Eirik Lygre <eirik.lygre@gmail.com> wrote:

=A0 // Functional, symbolic api. Changes are serialized immediately
=A0 Map<String, ORDER> getSort() =A0// Ret= urns immmutable, ordered map
=A0 setSort(Li= nkedHashMap<S,O>);
=A0 addSort(String= , ORDER);
=A0 removeSort();
=A0 removeSort(String);

<= div>
So, a couple of questions out of this:
- Do we want= to maintain backward compatibility =A0i.e. the use case with setting the s= tring and then manipulating it?
- Is there a clear point for serialization, so that we can serialize &= quot;at the end" only?

Two comments for the api:

1) There is a use case that might be fairly common, in which first a nu= mber of sorts are set, and then the user wants to change the direction of t= hat sort. Presumably this happens over time, and over several user interact= ions, 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 lik= e; 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.change= Sort("a", DESC); // Works, though it is not clear what to do if &= quot;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 &q= uot;clearSort()", with a preference to the latter (it matches "cl= ear()" which is used to clear the entire SolrQuery/ModifiableSolrParam= s)

// Functional, symbolic api. Changes are serialized imm= ediately
clearSort();
Map<String, ORDER> getSort() =A0// Returns immutable, ordered map=
setSort(LinkedHashMap<S,O>);
addSort(String, ORDER);
changeOrAddSort (String, ORDER)
remov= eSort(String);

<= /div>Attached is a patch; I'll put it up on jira once we're kinda h= appy with it :-)

1) I renamed the new method (#1 above) to "addOrUpdateSort()". Pr= obably not a particularly good parallel, but I found org.apache.lucene.inde= x.FieldInfos.addOrUpdate(), and reused that convention
2) I removed the new raw string methods getSortS= tring() and setSortString(). The point of this exercise is to use a symboli= c api, and if you want the raw field, use get()/set() with CommonParams.SOR= T.

The attached patch now contains a few fixes to the old api, tests for t= hose 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
--20cf3079b62ae589a004cf087b1b--