From 4ec9bd2e236907882d206c4b64efb29776291424 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 4 Jun 2024 13:43:14 +0200 Subject: [PATCH 1/4] Fix bug in filtering chunks that belong to a certain store range Signed-off-by: Christian Haudum --- pkg/storage/stores/composite_store_entry.go | 2 +- pkg/storage/stores/composite_store_test.go | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg/storage/stores/composite_store_entry.go b/pkg/storage/stores/composite_store_entry.go index 2c6e3c5e4a90c..935e061a74d83 100644 --- a/pkg/storage/stores/composite_store_entry.go +++ b/pkg/storage/stores/composite_store_entry.go @@ -90,7 +90,7 @@ func (c *storeEntry) GetChunks( func filterForTimeRange(refs []*logproto.ChunkRef, from, through model.Time) []chunk.Chunk { filtered := make([]chunk.Chunk, 0, len(refs)) for _, ref := range refs { - if through >= ref.From && from < ref.Through { + if (through >= ref.From && from < ref.Through) || (ref.From == from && ref.Through == from) { filtered = append(filtered, chunk.Chunk{ ChunkRef: *ref, }) diff --git a/pkg/storage/stores/composite_store_test.go b/pkg/storage/stores/composite_store_test.go index ab44f6b69d3c2..28052c528f7c2 100644 --- a/pkg/storage/stores/composite_store_test.go +++ b/pkg/storage/stores/composite_store_test.go @@ -421,6 +421,23 @@ func TestFilterForTimeRange(t *testing.T) { through: 7, exp: mkChks(5, 7), }, + { + desc: "ref with from == through", + input: []*logproto.ChunkRef{ + {From: 1, Through: 1}, // outside + {From: 2, Through: 2}, // ref.From == from == ref.Through + {From: 3, Through: 3}, // inside + {From: 4, Through: 4}, // ref.From == through == ref.Through + {From: 5, Through: 5}, // outside + }, + from: 2, + through: 4, + exp: []chunk.Chunk{ + {ChunkRef: logproto.ChunkRef{From: 2, Through: 2}}, + {ChunkRef: logproto.ChunkRef{From: 3, Through: 3}}, + {ChunkRef: logproto.ChunkRef{From: 4, Through: 4}}, + }, + }, } { t.Run(tc.desc, func(t *testing.T) { got := filterForTimeRange(tc.input, tc.from, tc.through) From f9b623e02bd271b1b63ed0c1a262f331a2a05c0f Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 4 Jun 2024 13:43:52 +0200 Subject: [PATCH 2/4] Do not shadow variable names Signed-off-by: Christian Haudum --- pkg/storage/stores/composite_store.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/storage/stores/composite_store.go b/pkg/storage/stores/composite_store.go index b975873c3043d..834d9602727fc 100644 --- a/pkg/storage/stores/composite_store.go +++ b/pkg/storage/stores/composite_store.go @@ -172,8 +172,8 @@ func (c CompositeStore) GetChunks( ) ([][]chunk.Chunk, []*fetcher.Fetcher, error) { chunkIDs := [][]chunk.Chunk{} fetchers := []*fetcher.Fetcher{} - err := c.forStores(ctx, from, through, func(innerCtx context.Context, from, through model.Time, store Store) error { - ids, fetcher, err := store.GetChunks(innerCtx, userID, from, through, predicate, storeChunksOverride) + err := c.forStores(ctx, from, through, func(innerCtx context.Context, innerFrom, innerThrough model.Time, store Store) error { + ids, fetcher, err := store.GetChunks(innerCtx, userID, innerFrom, innerThrough, predicate, storeChunksOverride) if err != nil { return err } From 9e3c4bc43fb5b6ee68da11bef5e1db59ccb8cb8e Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 4 Jun 2024 15:27:03 +0200 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Ed Welch --- pkg/storage/stores/composite_store_entry.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/storage/stores/composite_store_entry.go b/pkg/storage/stores/composite_store_entry.go index 935e061a74d83..24c5da0d1da15 100644 --- a/pkg/storage/stores/composite_store_entry.go +++ b/pkg/storage/stores/composite_store_entry.go @@ -90,7 +90,10 @@ func (c *storeEntry) GetChunks( func filterForTimeRange(refs []*logproto.ChunkRef, from, through model.Time) []chunk.Chunk { filtered := make([]chunk.Chunk, 0, len(refs)) for _, ref := range refs { - if (through >= ref.From && from < ref.Through) || (ref.From == from && ref.Through == from) { + // Only include chunks where the query start time (from) is < the chunk end time (ref.Through) + // and the query end time (through) is >= the chunk start time (ref.From) + // A special case also exists where a chunk can contain a single log line which results in ref.From being equal to ref.Through, but only include this chunk if this log line timestamp is in the timerange of the query + if (through >= ref.From && from < ref.Through) || (ref.From == ref.Through && (ref.From >= from && ref.From < through) { filtered = append(filtered, chunk.Chunk{ ChunkRef: *ref, }) From 6540c5cac9f0411a050d8434e7683afaff328b6c Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 4 Jun 2024 15:36:25 +0200 Subject: [PATCH 4/4] fixup! Apply suggestions from code review Signed-off-by: Christian Haudum --- pkg/storage/stores/composite_store_entry.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/storage/stores/composite_store_entry.go b/pkg/storage/stores/composite_store_entry.go index 24c5da0d1da15..5b8db237f4c33 100644 --- a/pkg/storage/stores/composite_store_entry.go +++ b/pkg/storage/stores/composite_store_entry.go @@ -92,8 +92,8 @@ func filterForTimeRange(refs []*logproto.ChunkRef, from, through model.Time) []c for _, ref := range refs { // Only include chunks where the query start time (from) is < the chunk end time (ref.Through) // and the query end time (through) is >= the chunk start time (ref.From) - // A special case also exists where a chunk can contain a single log line which results in ref.From being equal to ref.Through, but only include this chunk if this log line timestamp is in the timerange of the query - if (through >= ref.From && from < ref.Through) || (ref.From == ref.Through && (ref.From >= from && ref.From < through) { + // A special case also exists where a chunk can contain a single log line which results in ref.From being equal to ref.Through, and that is equal to the from time. + if (through >= ref.From && from < ref.Through) || (ref.From == from && ref.Through == from) { filtered = append(filtered, chunk.Chunk{ ChunkRef: *ref, })