blugelabs / bluge

indexing library for Go
Apache License 2.0
1.9k stars 125 forks source link

Aggregations only work with TopNSearch? #73

Open tmm1 opened 3 years ago

tmm1 commented 3 years ago

If I use bluge.NewAllMatches and add aggregations, no data is ever returned in the buckets.

This reproduces the failure in the test suite:

diff --git a/test/aggregations_test.go b/test/aggregations_test.go
index bddc664..41a3cdd 100644
--- a/test/aggregations_test.go
+++ b/test/aggregations_test.go
@@ -223,7 +223,7 @@ func aggregationsTests() []*RequestVerify {
        return []*RequestVerify{
                {
                        Comment: "category inventory, by type",
-                       Request: bluge.NewTopNSearch(0,
+                       Request: bluge.NewAllMatches(
                                bluge.NewTermQuery("inventory").
                                        SetField("category")),
                        Aggregations: search.Aggregations{

Is this expected behavior?

mschoch commented 3 years ago

First, I have not reviewed the code, so possibly it is just broken.

But, I think the integration test was not written in a way that it works by seamlessly switching from TopN to All. The reason is that when using the TopN search, as soon as the Search() method returns, the aggregations have already been calculated. However, when using the All search, the aggregations are computed WHILE you call Next() on the DocumentMatchIterator. Now, if the test had filled in the ExpectedMatches section, the test framework would have iterated through them, but the original test had size set to 0, so it didn't specify what the results were going to look like. In summary, in order for this test to be valid after switching to NewAllMatches, we would have to populate the ExpectedResults section, and it would have to contain all the results (because we used All Matches). Whether or not this test passes or fails at that point, I'm still not sure...

tmm1 commented 3 years ago

when using the All search, the aggregations are computed WHILE you call Next() on the DocumentMatchIterator.

Thanks, this explains why it appeared to not work in my app.

I changed the test to try and demonstrate the behavior I was seeing. I guess I got lucky the test didn't iterate either.

Some examples of aggregation would be helpful in the docs. I've been trying to piece it together from the test cases, but there's a lot of abstraction in the test suite.