Split HasChildQueryParser into toQuery and formXContent#13333
Split HasChildQueryParser into toQuery and formXContent#13333s1monw merged 1 commit intoelastic:feature/query-refactoringfrom
Conversation
There was a problem hiding this comment.
I think the type and queryBuilder instance members could be made final
There was a problem hiding this comment.
I'd love to make all the things here final and remove that builder stuff but maybe we do this later?
There was a problem hiding this comment.
yes lets do it later otherwise we have to remove setters and break things.
There was a problem hiding this comment.
but that's the point of this refactoring right? I am not saying we should now but we have to
There was a problem hiding this comment.
sure things changes now that we know for sure the target branch, that said making everything final would be better to do once we merged back to master to prevent merge conflicts here. Same with renaming XYZQueryBuilder to XYZQuery, and moving to proper getters and setters.
|
I didi a first review round, especially on the query refactoring aspects, and left some comments. I'll have to dive a bit deeper into the inner_hits changes to better understand it. |
There was a problem hiding this comment.
I thought the default was 0 in the previous parser, wasn't it? did you make this change on purpose?
There was a problem hiding this comment.
yeah which was then turned into MaxINt on the server
There was a problem hiding this comment.
I can go back to the previous
There was a problem hiding this comment.
I see! I think your change makes it clearer actually, looks good
887ca3c to
ed3a719
Compare
|
@javanna I think it's ready can you give it another go |
|
@cbuescher since luca is out can you take a look? |
There was a problem hiding this comment.
In some previous discussion for other builders we tried to set default values for non primitive types that have a default, setting to null effectively means use the default, same as specifying field: null in the json query. Maybe we can keep this here (and in the following setters that have default values)
There was a problem hiding this comment.
if you want to set this to default you shouldn't pass null to this. Passing null makes no sense here at all you can just pass None and passing null is 100% of the time a bug
There was a problem hiding this comment.
Okay, we might have to revisit some other queries we refactored then.
There was a problem hiding this comment.
I think we can simplify here and print everything out, default values included, that's what we went for in all of the other queries too.
|
Added one more minor comment, LGTM otherwise. |
There was a problem hiding this comment.
I do like this change, it simplifies things a lot. What I am wondering though is if it was simply wrapping a search source, or also hiding that inner_hits don't support everything that can be put within a search source. For instance, the query element doesn't seem to be supported, as well as other parse elements I'm afraid. This becomes a problem in the java api only I think, cause you can potentially set things that aren't supported. Maybe it is ok for now....
There was a problem hiding this comment.
yeah I think we should just let it be like this for now and fix it along the way. I had it as a TODO on some calsses but maybe it's too much for now?
This is an intial commit that splits HasChildQueryParser / Builder into
the two seperate steps. This one is particularly nasty since it transports
a pretty wild InnerHits object that needs heavy refactoring. Yet, this commit
has still some nocommits and needs more tests and maybe another cleanup but
it's a start to get the code out there.