Skip to content

Commit 54313dd

Browse files
committed
fix(nginx_log): correct Total handling when merging facets
1 parent f2f5a08 commit 54313dd

File tree

2 files changed

+124
-1
lines changed

2 files changed

+124
-1
lines changed

internal/nginx_log/searcher/facet_aggregator.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ func (ds *DistributedSearcher) mergeFacets(combined, incoming map[string]*Facet)
5050

5151
// mergeSingleFacet merges two facets for the same field
5252
func (ds *DistributedSearcher) mergeSingleFacet(existing, incoming *Facet) {
53-
existing.Total += incoming.Total
53+
// Note: Do NOT sum Total values - it represents unique terms count, not document count
54+
// The Total should be recalculated based on the actual number of unique terms after merging
5455
existing.Missing += incoming.Missing
5556
existing.Other += incoming.Other
5657

@@ -96,6 +97,8 @@ func (ds *DistributedSearcher) mergeSingleFacet(existing, incoming *Facet) {
9697
}
9798

9899
existing.Terms = terms
100+
// Set Total to the actual number of unique terms (not sum of totals)
101+
existing.Total = len(termCounts)
99102
}
100103

101104
// copyFacet creates a deep copy of a facet
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
package searcher
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
func TestMergeSingleFacet_UniqueTermsCount(t *testing.T) {
10+
ds := &DistributedSearcher{}
11+
12+
// Create initial facet with some terms from shard 1
13+
existing := &Facet{
14+
Field: "path_exact",
15+
Total: 3, // Initial count of unique terms
16+
Missing: 5,
17+
Other: 10,
18+
Terms: []*FacetTerm{
19+
{Term: "/api/users", Count: 100},
20+
{Term: "/api/posts", Count: 80},
21+
{Term: "/home", Count: 60},
22+
},
23+
}
24+
25+
// Create incoming facet from shard 2
26+
// Has some overlapping terms and some new terms
27+
incoming := &Facet{
28+
Field: "path_exact",
29+
Total: 4, // Should NOT be added to existing.Total
30+
Missing: 3,
31+
Other: 8,
32+
Terms: []*FacetTerm{
33+
{Term: "/api/users", Count: 50}, // Overlapping
34+
{Term: "/api/posts", Count: 30}, // Overlapping
35+
{Term: "/login", Count: 40}, // New
36+
{Term: "/dashboard", Count: 35}, // New
37+
},
38+
}
39+
40+
// Merge the facets
41+
ds.mergeSingleFacet(existing, incoming)
42+
43+
// Verify the results
44+
assert.Equal(t, "path_exact", existing.Field)
45+
46+
// Total should be the count of unique terms after merging, NOT the sum
47+
// We have 5 unique terms: /api/users, /api/posts, /home, /login, /dashboard
48+
assert.Equal(t, 5, existing.Total, "Total should be the count of unique terms, not sum of totals")
49+
50+
// Missing and Other should be summed
51+
assert.Equal(t, 8, existing.Missing)
52+
assert.Equal(t, 18, existing.Other)
53+
54+
// Verify term counts are merged correctly
55+
termMap := make(map[string]int)
56+
for _, term := range existing.Terms {
57+
termMap[term.Term] = term.Count
58+
}
59+
60+
assert.Equal(t, 150, termMap["/api/users"], "Count should be 100+50")
61+
assert.Equal(t, 110, termMap["/api/posts"], "Count should be 80+30")
62+
assert.Equal(t, 60, termMap["/home"], "Count should remain 60")
63+
assert.Equal(t, 40, termMap["/login"], "Count should be 40")
64+
assert.Equal(t, 35, termMap["/dashboard"], "Count should be 35")
65+
}
66+
67+
func TestMergeSingleFacet_WithLimitAndOther(t *testing.T) {
68+
// DefaultFacetSize is 10 by default, so we'll use enough terms to exceed it
69+
ds := &DistributedSearcher{}
70+
71+
existing := &Facet{
72+
Field: "path",
73+
Total: 5,
74+
Missing: 0,
75+
Other: 0,
76+
Terms: []*FacetTerm{
77+
{Term: "/page1", Count: 1000},
78+
{Term: "/page2", Count: 900},
79+
{Term: "/page3", Count: 800},
80+
{Term: "/page4", Count: 700},
81+
{Term: "/page5", Count: 600},
82+
},
83+
}
84+
85+
incoming := &Facet{
86+
Field: "path",
87+
Total: 8,
88+
Missing: 0,
89+
Other: 0,
90+
Terms: []*FacetTerm{
91+
{Term: "/page1", Count: 500},
92+
{Term: "/page6", Count: 550},
93+
{Term: "/page7", Count: 450},
94+
{Term: "/page8", Count: 400},
95+
{Term: "/page9", Count: 350},
96+
{Term: "/page10", Count: 300},
97+
{Term: "/page11", Count: 250},
98+
{Term: "/page12", Count: 200},
99+
},
100+
}
101+
102+
ds.mergeSingleFacet(existing, incoming)
103+
104+
// Total should be 12 unique paths (page1-5 from existing, page6-12 from incoming)
105+
assert.Equal(t, 12, existing.Total, "Should have 12 unique paths")
106+
107+
// DefaultFacetSize is 10, so we should keep top 10 terms
108+
assert.Equal(t, 10, len(existing.Terms), "Should keep top 10 terms (DefaultFacetSize)")
109+
110+
// Verify top terms are correct
111+
assert.Equal(t, "/page1", existing.Terms[0].Term)
112+
assert.Equal(t, 1500, existing.Terms[0].Count) // 1000 + 500
113+
assert.Equal(t, "/page2", existing.Terms[1].Term)
114+
assert.Equal(t, 900, existing.Terms[1].Count)
115+
assert.Equal(t, "/page3", existing.Terms[2].Term)
116+
assert.Equal(t, 800, existing.Terms[2].Count)
117+
118+
// Other should contain the sum of excluded terms (page11: 250, page12: 200)
119+
assert.Equal(t, 450, existing.Other, "Other should contain sum of excluded terms")
120+
}

0 commit comments

Comments
 (0)