Query refactoring: SpanOrQueryBuilder and Parser #12342
Conversation
cbuescher
changed the title
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| public class SpanOrQueryBuilder extends AbstractQueryBuilder<SpanOrQueryBuilder> implements SpanQueryBuilder<SpanOrQueryBuilder> { |
There was a problem hiding this comment.
Nitpick: With this turning into a public API - should we have some class level doc here?
|
Left some minor comments. |
|
@MaineC thanks for the review, just pushed new commit addressing your comments. Mind to have another look? |
|
LGTM - I'm sure @javanna will will find a few more areas for improvment |
| SpanQuery[] spanQueries = new SpanQuery[clauses.size()]; | ||
| for (int i = 0; i < clauses.size(); i++) { | ||
| Query query = clauses.get(i).toQuery(parseContext); | ||
| assert query instanceof SpanQuery; |
There was a problem hiding this comment.
we might have the same problem with LateParsingQuery here too right? then this should become an if instead? I guess we might have the same somewhere else too?
There was a problem hiding this comment.
Not really, all query clauses here need to be SpanQueryBuilders, the problem with LateParsingQuery was related to MultiTermQueryBuilders (which RangeQuery is a subtype of). We are creating SpanOrQuery here which takes list of SpanQueries as an argument, so all good I think.
There was a problem hiding this comment.
ok sorry for the confusion, here and in the other PR where I commented...
|
left one small comment |
cbuescher
force-pushed
the
feature/query-refactoring-spanor
branch
from
674f43f to
d5954d6
Compare
|
Addressed comments and removed check for empty clauses in doXContent since this will either be caught by parser or in validate. Left one question regarding tests to get another opinion from @MaineC. |
cbuescher
force-pushed
the
feature/query-refactoring-spanor
branch
from
4bc2b9f to
c6f24ae
Compare
Moving the query building functionality from the parser to the builders new toQuery() method analogous to other recent query refactorings. Relates to elastic#10217
…n amount of queries with same fieldname
cbuescher
force-pushed
the
feature/query-refactoring-spanor
branch
from
c6f24ae to
8e20536
Compare
|
After checking the modifications in person with @cbuescher this morning: Looks all good to me. |
…anor Query refactoring: SpanOrQueryBuilder and Parser
…ring-spanor Query refactoring: SpanOrQueryBuilder and Parser
clintongormley
added
:Search/Search
Moving the query building functionality from the parser to the builders
new doToQuery() method analogous to other recent query refactorings.
Relates to #10217