Query refactoring: SpanOrQueryBuilder and Parser#12342
Query refactoring: SpanOrQueryBuilder and Parser#12342cbuescher merged 4 commits intoelastic:feature/query-refactoringfrom
Conversation
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 |
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 |
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. |
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
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
Moving the query building functionality from the parser to the builders
new doToQuery() method analogous to other recent query refactorings.
Relates to #10217