Skip to content

Conversation

javanna
Copy link
Member

@javanna javanna commented Jun 26, 2015

Added validation for all inner queries that any already refactored query may hold. Added also tests around this. At the end of the refactoring validate will be called by SearchRequest#validate during TransportSearchAction execution, which will call validate against the top level query builder that will need to go and validate its data plus all of its inner queries, and so on.

@javanna javanna force-pushed the enhancement/query_validate branch from c389ff4 to 7d2a0e9 Compare June 26, 2015 14:13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how this gives context to the error message, wondering if there is an easy way to be more specific in case there is more than query with the same name in the complete query (like a match in both must and should clause of BooleanQueryBuilder). I guess that's not possible right now, but would be great if inner queries knew about their parents, then we could output the full path here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, this makes sense. I just checked what we currently do given that validation before refactoring happens while parsing. No context is given for messages that we can move to validate, which are not json specific. That means this is not a regression caused by the refactoring, rather an improvement that we could work on in the future ;)

@cbuescher
Copy link
Member

I went through the changes quickly, it is great that we can validate recursively that way. LGTM besides the small remark which at the moment doesn't seem solvable, unless you have an idea. Tracking the path in the query tree as state in some collector object we pass in with the topmost query might be possible, but maybe overkill at this point.

@javanna
Copy link
Member Author

javanna commented Jun 30, 2015

LGTM besides the small remark which at the moment doesn't seem solvable, unless you have an idea.

I would leave keeping track of the query tree etc. for later, it is a nice to have though, opened #11939 for that.

@javanna javanna force-pushed the enhancement/query_validate branch 2 times, most recently from b62c466 to 1861095 Compare June 30, 2015 12:13
Added validation for all inner queries that any already refactored query may hold. Added also tests around this. At the end of the refactoring validate will be called by SearchRequest#validate during TransportSearchAction execution, which will call validate against the top level query builder that will need to go and validate its data plus all of its inner queries, and so on.

Closes elastic#11889
@javanna javanna force-pushed the enhancement/query_validate branch from 1861095 to 10f8016 Compare June 30, 2015 12:25
@javanna javanna merged commit 10f8016 into elastic:feature/query-refactoring Jun 30, 2015
@kevinkluge kevinkluge removed the review label Jun 30, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
Added validation for all inner queries that any already refactored query may hold. Added also tests around this. At the end of the refactoring validate will be called by SearchRequest#validate during TransportSearchAction execution, which will call validate against the top level query builder that will need to go and validate its data plus all of its inner queries, and so on.

Closes elastic#11889
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants