-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
FiltersAggregation support breaks extraction of other aggregations #1181
Comments
Gotta take a look at this, thanks for the tip! |
@amironoff any chance you could provide an actual example? I haven't been able to reproduce this myself. Here's what I've tried: var response = this.Client.Search<ElasticsearchProject>(s => s
.Aggregations(a => a
.Terms("my-terms", t => t
.Field(p => p.Name)
)
.DateRange("my-date-range", dr => dr
.Field(p => p.StartedOn)
.Ranges(
rs => rs.From("19000101").To("now"),
rs => rs.From("19500101").To("now")
)
)
.Filters("my-filters", fs => fs
.Filters(
ffs => ffs.Term(p => p.Country, "Malaysia"),
ffs => ffs.Term(p => p.Name, "elasticsearch")
)
)
)
);
response.IsValid.Should().BeTrue();
var myTerms = response.Aggs.Terms("my-terms");
myTerms.Should().NotBeNull();
myTerms.Items.Count.Should().BeGreaterThan(0);
var myDateRange = response.Aggs.DateRange("my-date-range");
myDateRange.Should().NotBeNull();
myDateRange.Items.Count.Should().BeGreaterThan(0);
var myFilters = response.Aggs.Filters("my-filters");
myFilters.Should().NotBeNull();
myFilters.Items.Count().Should().Be(2); All passes. @radiosterne have you looked into this at all yet and maybe have had some luck reproducing it? |
@gmarz Okay, I'll try to compile a full example. The ES version I'm using is 1.4.0. Is "this.Client" in your example a mock or an actual ES client? |
@amironoff great, thanks. It's an actual ES client. |
Hey @amironoff any luck in producing an example yet? |
@gmarz every example I've tried works good, so I haven't been able to reproduce; waiting for @amironoff |
Thanks @radiosterne ! The aggs deserialization code being as brittle as it is, I would't be surprised at all if there is some edge case that @amironoff ran into that breaks it. |
I absolutely agree, @gmarz, with our current implementation of stateless deserialization I think, actually, there are plenty of such cases :) |
Hi guys! The example is coming! Interestingly, ability to reproduce depends on the aggregation key and order of registering the aggs. More to come soon! :) |
Okay, please find a full example in https://github.com/amironoff/es-aggregation-bug/tree/master |
Wow @amironoff, this is great! I was expecting a simple test case, but you went the extra mile. This will make things much easier, thank you. Looking into this now, stay tuned. |
@amironoff @radiosterne This ended up being another case of the aggregation response not being read to completion due to ordering of the results (similar to #1169 and #931). This only occurred when the filters portion of the response was returned first, which is why we had a hard time reproducing it. Pushed the above fix and it'll be available in the next release. Thanks again @amironoff for the awesome repro. 👍 |
FiltersAggregation support, introduced in #1121, breaks extraction of other aggregations from ES response. This affects the latest version in development (Tested on 1.4.0-ci20150102152352). Steps to reproduce:
The text was updated successfully, but these errors were encountered: