-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse Failure: NullPointerException #8438
Comments
Agreed - can you tell us what you were doing when you saw this error? What does the query/agg look like? |
It was mostly simple terms aggs but also one global agg with a filter. IIRC the query itself was that same filter being applied to the global agg. |
@mausch could you provide some examples? Clearly we're not seeing this in our testing, so it would be helpful to see what you are doing different. |
Sorry, I don't have that query any more. But I still insist that it shouldn't matter. I'd run some random-testing tool (e.g. https://github.com/pholser/junit-quickcheck ) on |
@mausch I agree with you - we don't wilfully leave NPEs in our code. The trick is finding out what is causing it. |
I am new to this community and I think I can fix this one to start with. Has some one already fixed it ? |
@clintongormley Indeed, especially with complex code. Sorry I lost the original query. The only thing I can advice is to start applying randomized testing (if you haven't already, I don't know much about the ES codebase). It's great to find this kind of bugs. IIRC Lucene has started using it a couple of years ago. @vaibhavkulkar unless there's already some randomized testing in place in the project this isn't trivial to fix. |
can you tell what filters you are using? |
IIRC it was an And filter with a single terms filter within. |
@mausch also, what version of Elasticsearch did you see this on? |
The filters aggregation should in at FilterParser.java line 41 check for null. A null value is a valid return value for a filter parser (and for example the |
@clintongormley version 1.3.4 |
I found the same NPE. FilterBuilder fb = new AndFilterBuilder();
FilterAggregationBuilder fab = AggregationBuilders
.filter("filter")
.filter(fb)
.subAggregation(AggregationBuilders.terms("brandId").field("brand_id").size(0))
.subAggregation(AggregationBuilders.terms("allAttr").field("all_attr").size(0)); |
@martijnvg I can see where you say check for null but do you believe the correct response is then to throw a parse exception with a friendly message or to process the query using something like a MatchNoDocsFilter? |
@markharwood I think if a filter parser returns null then the filters agg should interpret this as nothing is going to match, so |
We had the same issue with bool query and what we do there is: if (clauses.isEmpty()) {
return new MatchAllDocsQuery();
} I think bool filter should be consistent here - I am not sure which one we should change to be honest... |
Tough one. My gut feel on the empty filters array would be that it would mean "match none" but if we have a precedent for this on the query side which is "match all" then I guess we have these ugly options:
|
The If we don't specify a |
+1 for what clint said. I thinks the deal here is |
…empty. Added Junit test that recreates the error and fixed FilterParser to default to using a MatchAllDocsFilter if the requested filter clause is left empty. Also added fix and test for the Filters (with an "s") aggregation. Closes #8438
…empty. Added Junit test that recreates the error and fixed FilterParser to default to using a MatchAllDocsFilter if the requested filter clause is left empty. Also added fix and test for the Filters (with an "s") aggregation. Closes #8438
…empty. Added Junit test that recreates the error and fixed FilterParser to default to using a MatchAllDocsFilter if the requested filter clause is left empty. Also added fix and test for the Filters (with an "s") aggregation. Closes #8438
…empty. Added Junit test that recreates the error and fixed FilterParser to default to using a MatchAllDocsFilter if the requested filter clause is left empty. Also added fix and test for the Filters (with an "s") aggregation. Closes elastic#8438
…empty. Added Junit test that recreates the error and fixed FilterParser to default to using a MatchAllDocsFilter if the requested filter clause is left empty. Also added fix and test for the Filters (with an "s") aggregation. Closes elastic#8438
Query is pretty big and I have no idea what's causing it. I don't think that's relevant anyway, IMHO the code should check for nulls and throw a more specific exception, otherwise we clients are left in the dark about what we're doing wrong.
Here's the stack trace:
The text was updated successfully, but these errors were encountered: