Query refactoring: FieldMaskingSpanQueryBuilder and Parser

[cbuescher]

Moving the query building functionality from the parser to the builders
new toQuery() method analogous to other recent query refactorings.

Relates to #10217

[javanna]

but why? :)

[cbuescher]

oh my! :)

[javanna]

left a few comments

[javanna]

I see your point now, I am confused, I thought SpanQueryBuilder was an abstract class extending QueryBuilder...I see that's a difference between master and our branch. Maybe some merge went wrong?

[cbuescher]

Will check how the inheritance hierarchy looks like on master and revisit this.

[cbuescher]

The reason for this is that there are two QueryBuilder and SpanQueryBuilders on the classpath:

• (interface) lucene-SpanQueryBuilder extends (interface) lucene-QueryBuilder
• FieldMaskingSpanQB implements es-SpanQueryBuilder (a marker interface)

We are talking about the later here. In adition we need to cast a lucene Query to a lucene SpanQuery here. I think the core problem is that SpanQuerybuilder (the ES-interface) is just a marker and we can't be sure everything implementing it is a ES-QueryBuilder. But that's not so easily fixed here.

[cbuescher]

Scratch that previous comment, I see what you mean now, will see where that comes from on master.

[cbuescher]

Okay, this is getting a bit complicted now:
I re-checked to see now that back in may you made SpanQueryBuilder an abstract class extending QueryBuilder on master, but during the refactoring of the SpanTermQuery (#11005) this was changed back to be an interface on our feature branch. I tried to change it back to be an abstract class like on master, but now I have trouble with the inheritance hierarchy of SpanTermQuery. Here we have:

• SpanTermQueryBuilder extends BaseTermQueryBuilder<SpanTermQueryBuilder>
• TermQueryBuilder extends BaseTermQueryBuilder<TermQueryBuilder>
• abstract class BaseTermQueryBuilder<QB extends BaseTermQueryBuilder<QB>> extends QueryBuilder<QB>

I'm having trouble now fitting the SpanQueryBuilder in here. If I merge it in the way it is on master it sits between all the other Span*Q.B. and QueryBuilder, but for the SpanTermQB I cannot easily make it extend BaseTermQueryBuilder. Maybe I'm missing something here. Maybe this was the reason it was changed back in #11005 to be a marker interface?

[javanna]

it's fine, when I did the refactoring SpanQueryBuilder became an abstract class without any problem, but now it needs to be a marker interface again cause java doesn't support multiple inheritance ;) We just need a cast, sorry for the noise I had missed this change to be honest but now I get it, thanks a lot for digging!

[javanna]

Thinking more about it, it is probably more natural to make your proposed change and have the QueryBuilder member. Then also update the constructor to:

public <T extends QueryBuilder & SpanQueryBuilder> FieldMaskingSpanQueryBuilder(T queryBuilder, String field) {

so we make sure that the argument is both a query builder and a span query builder. Thoughts?

[cbuescher]

Thanks, yes I think I prefer the QueryBuilder member, after all the interface is just a marker and we call toQuery on the inner object. I tried your sugesstion with the bound type parameter in the constructor, but then I couldn't use any general QueryBuilder or SpanQueryBuilder type when calling that constructor, like in:

QueryBuilder innerQueryBuilder = in.readNamedWriteable();
new FieldMaskingSpanQueryBuilder(innerQueryBuilder, in.readString());

I changed it back to like it was before unless you have another idea?

[cbuescher]

@javanna Changed back to use QueryBuilder for inner queries, looked at your generics solution for the constructor but somehow didn't get that to work.

[cbuescher]

What if we add SpanQuery toQuery() to the SpanQueryBuilder interface? That way we actually reflect the only important distinction of span queries and can drop most of the unsafe casting. I added a commit that show what I mean. Notice that the extra toQuery() methods I had to introduce to the remaining span queries will go away once we refactor them.

[cbuescher]

Rebased and updated PR to reflect latest changes in QueryBuilder inheritance hierarchy.

[javanna]

what naming convention do we want to follow here? I saw around some innerQueryBuider(), here innerQuery(), maybe we even want to drop the inner prefix... let's decide for one of the options and go for it everywhere.

[cbuescher]

Good question. I just checked, in BoostingQB we use QueryBuilder positive() and QueryBuilder negative(), but that was mainly because there were setters called positiv(QB) and negative(QB). In ConstantScoreQB the getter is simply query(). In the QueryFilterBuilder PR I use innerQueryBuilder() but since at some point we might move away from the builders for the intermediate query representation, I'd rather change that there as well.

So I'd vote for innerQuery() or simply query() if there is only one query that is beeing wrapped. No strong preferences there, maybe leaning to innerQuery() because when using it like newFilterQuery.innerQuery() it's clearer to me that the output doesn't refer to the parent query itself.

[javanna]

all sounds good to me. innerQuery is fine.

[javanna]

LGTM besides the small comment I left