Parse has_child query/filter after child type has been parsed #5838
Conversation
| } finally { | ||
| QueryParseContext.setTypes(origTypes); | ||
| } | ||
| innerQueryBytes = XContentFactory.jsonBuilder().map(parser.mapOrdered()).bytes(); |
There was a problem hiding this comment.
we can do this in a more optimize manner. We can create a builder, and then use copyCurrentStructure on the builder (passing it a parser), to just copy it over, compared with parsing into a map and then serializing the map. Also, since its internal, I would use smile builder, as its considerably more efficient than json.
|
can we have a test that verifies it by sending building the search request programmatically maybe (and not using the query builders)? |
Whoops! Forgot to stage the file with the test, pushed with the test and builder change |
| } finally { | ||
| QueryParseContext.setTypes(origTypes); | ||
| } | ||
| innerQueryBytes = XContentFactory.smileBuilder().copyCurrentStructure(parser).bytes(); |
There was a problem hiding this comment.
this is not guaranteed to be smile! We randomize this in our tests I thing this should break if you run it multiple times.
There was a problem hiding this comment.
This isn't parsing this as smile, it's creating a internal parser that can be read from. I used smile because Shay recommended it as more efficient than a JSON builder.
There was a problem hiding this comment.
oh I misread! I wonder if we need this somewhere else and if we could represent this as an object ie something like this:
public class XContentStructure {
public XContentStructure( XContentParser parser, WhaterverWeNeedHere ) {
// copy current struct...
}
public Query asQuery(String...types) {
//... do it
}
public Filter asFilter(String...types) {
//... do it
}
}
does this make sense?
|
while you are at it maybe we want to fix the |
|
Also the |
|
Sure, I will work on fixes for the |
|
@s1monw @martijnvg pushed a new commit addressing the other query types and refactoring. |
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.common.xcontent; |
There was a problem hiding this comment.
since this deals with parsing of query/filter, I don't think it should be in the common.xcontent package, maybe place it in the same package as QueryParserContext?
There was a problem hiding this comment.
I think org.elasticsearch.index.query contains almost entirely builder/parsers and QueryParseContext, I think org.elasticsearch.index.query.support would fit better
|
@martijnvg added some commits to this, it now will parse the query/filter in a streaming manner if they types are available when the query/filter is encountered. |
|
Thanks @dakrone! LGTM |
Fixes #5783 Fixes #5838 Conflicts: src/main/java/org/elasticsearch/index/query/HasChildFilterParser.java src/main/java/org/elasticsearch/index/query/HasChildQueryParser.java src/main/java/org/elasticsearch/index/query/HasParentFilterParser.java src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java src/main/java/org/elasticsearch/index/query/TopChildrenQueryParser.java
Fixes elastic#5783 Fixes elastic#5838 Conflicts: src/main/java/org/elasticsearch/index/query/HasChildFilterParser.java src/main/java/org/elasticsearch/index/query/HasChildQueryParser.java src/main/java/org/elasticsearch/index/query/HasParentFilterParser.java src/main/java/org/elasticsearch/index/query/HasParentQueryParser.java src/main/java/org/elasticsearch/index/query/TopChildrenQueryParser.java
clintongormley
added
the
:Search/Search
Fixes #5783