Improve API of PhraseQuery.Builder [LUCENE-6575]

[asfimport]

From #7589
In current PhraseQuery.Builder API. User must retype field again and again :

PhraseQuery.Builder builder = new PhraseQuery.Builder();
builder.add(new Term("lyrics", "when"), 1);
builder.add(new Term("lyrics", "believe"), 3);

Cleaner API :

PhraseQuery.Builder builder = new PhraseQuery.Builder("lyrics");
builder.add("when", 1);
builder.add("believe", 3);

---

Migrated from LUCENE-6575 by Cao Manh Dat (@CaoManhDat), updated Jun 22 2015
Attachments: LUCENE-6575.patch (versions: 5)

[asfimport]

Cao Manh Dat (@CaoManhDat) (migrated from JIRA)

Here is patch for this issue.

[asfimport]

Cao Manh Dat (@CaoManhDat) (migrated from JIRA)

Change javadocs.

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Thanks for the patch. I like providing providing the field in the constructor and then adding terms better. I have some questions:

• could you change the setters back to void so that they can't be chained, see https://issues.apache.org/jira/browse/LUCENE-6531?focusedCommentId=14577421&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14577421
• I don't like the two Builder(Term[]) and Builder(Term[], int[]) constructors. In my opinion they defeat the point of the builder which is to build the query step by step instead of all at once, can we remove them? They seem to be used only in very few places, could we fix the consumers of these constructors to use the builder API instead?

[asfimport]

Cao Manh Dat (@CaoManhDat) (migrated from JIRA)

@jpountz Thanks you for you comment.

• I agree that chaining in builder make it quite hard to read. I will fix that soon.
• About two new constructors of Builder. Currently, PhraseQuery act as BooleanQuery some times (it rely on terms.length() == 0 to rewrite to BooleanQuery). So consumer will not have to care about their terms[] is empty or not, they simply deliver it to PhraseQuery. If we remove these constructors of Builder, consumer will have to check it terms[] size and do the same thing. Is it approciate?

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

If we remove these constructors of Builder, consumer will have to check it terms[] size and do the same thing. Is it approciate?

I think that's fine.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

could you change the setters back to void so that they can't be chained, see https://issues.apache.org/jira/browse/LUCENE-6531?focusedCommentId=14577421&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14577421

I disagree with this. You should still give users who want to use call chaining to do this. If you don't want it you can use those methods like if they were void.

From my perspective, I have the problem that I cannot really read those chains of localVar.setXXX() because of the redundancy. Others see this different. But by making them to return this instead of void, you have both options.

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

I don't really have an opinion about whether allowing chaining should be allowed or not. Robert expressed concerns about chaining in #7589 and I didn't mind so I just made all setters void in the patch on #7589. And since Cao changed the setters to return "this" in his patch, I pointed him to this previous discussion.

We are not there yet, but please let's not put this chaining issue in the way of making our queries immutable.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

We are not there yet, but please let's not put this chaining issue in the way of making our queries immutable.

This issue was about improving the API, so it is relevant here. I think making phrase queries immutable is another thing and was discussed in #7589.

And since Cao changed the setters to return "this" in his patch, I pointed him to this previous discussion.

Robert's main problem was not the chaining, he disagreed with the "fluent" method names (Java 8+ streams API is using them everywhere now)!

But just returning "this" from a setter is in my opinion a valid simplification for users of the API. It mainly works around the problem of not having a "with" operator in java, like Javascript has:

with (object) {
 setA();
 setB();
}

On the Java side, recent versions of Eclipse perfectly detect & ident those chaining constructs (because Java 8 requires them to do this, otherwise code gets indeed unreadable).

I also added builder APIs in recently: CustomAnalyzer.builder() (see my talk @ buzzwords; http://lucene.apache.org/core/5_2_1/analyzers-common/org/apache/lucene/analysis/custom/CustomAnalyzer.html) and Robert did not complain, because the names were according to fluent API standard and the formatting in the tests was also fine :-)

[asfimport]

Cao Manh Dat (@CaoManhDat) (migrated from JIRA)

So should we keep chaining or not?

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

I suggest that we wait for @rmuir's opinion.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Uwe said it right. Look at his patch: #7239.patch

Uwe's tests are like this, each thing on its own line, and readable:

StringBuilder sb = new StringBuilder()
    .append(xxx)
    .append(yyy);

this is also fine:

StringBuilder sb = new StringBuilder();
sb.append(xxx);
sb.append(yyy);

on the other hand this is terrible:

StringBuilder sb = new StringBuilder().append(xxx).append(yyy)...

As far as concerns about method names, same situation there:
Please keep verbs like get and set, it may seem old fashioned but it makes code readable. It gives important hints about what the method is doing. You know instantly from the name a little bit more about what is happening without having to read source code.

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Thanks Robert, then let's keep the return statements like they are in Cao's patch.

[asfimport]

Cao Manh Dat (@CaoManhDat) (migrated from JIRA)

Thanks Robert,Andrien and Uwe. I will submit a patch in couple of hours.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Thanks Robert for the confirmation! This is how I understood your comment. I agree with having a clear method name. I am fine with pure builders to have "verbs" as names, but as soon as it is a complex object which also have getters and other state changing stuff, be explicit.

[asfimport]

Cao Manh Dat (@CaoManhDat) (migrated from JIRA)

Here is the patch based on suggestions. Because I found a lot of place do the same thing below

PhraseQuery pq = (PhraseQuery) query;
Term[] terms = pq.getTerms();
int[] positions = pq.getPositions();
PhraseQuery.Builder builder = new PhraseQuery.Builder();
for (int i = 0; i < terms.length; ++i) {
  builder.add(terms[i], positions[i]);
}
builder.setSlop(slop);
query = builder.build();
query.setBoost(pq.getBoost());

So i added a new Builder(PhraseQuery pq) constructor to reduce repeated works.