refactor: Pass query plan down to bloom gateway #12037
Conversation
salvacorts
force-pushed
the
salvacorts/pass-plan-to-boom-gateway
branch
from
d631fec
to
488f264
Compare
| Matchers []*labels.Matcher | ||
| Filters []syntax.LineFilter | ||
| plan *plan.QueryPlan |
There was a problem hiding this comment.
Matchers and plan now contain redundant information. The plan already contains the matchers.
There was a problem hiding this comment.
As discussed in Slack. We will leave as is right now. We can consider refactoring that on a separate PR.
pkg/querier/plan/plan.go
Outdated
| @@ -8,6 +8,8 @@ import ( | |||
|
|
|||
| type QueryPlan struct { | |||
| AST syntax.Expr | |||
|
|
|||
| lineFilterExprs *[]syntax.LineFilterExpr | |||
There was a problem hiding this comment.
QueryPlan should not contain individual parts of the AST separately.
There was a problem hiding this comment.
related: #12037 (comment)
pkg/storage/stores/shipper/indexshipper/indexgateway/gateway.go
Outdated
Show resolved
Hide resolved
pkg/bloomgateway/bloomgateway.go
Outdated
| @@ -334,7 +335,7 @@ func (g *Gateway) FilterChunkRefs(ctx context.Context, req *logproto.FilterChunk | |||
|
|
|||
| tasks := make([]Task, 0, len(seriesByDay)) | |||
| for _, seriesWithBounds := range seriesByDay { | |||
| task, err := NewTask(ctx, tenantID, seriesWithBounds, req.Plan.LineFilterExprs()) | |||
| task, err := NewTask(ctx, tenantID, seriesWithBounds, syntax.ExtractLineFilters(req.Plan.AST)) | |||
There was a problem hiding this comment.
If you want to avoid traversing the AST multiple times, you could extract the filters once in the beginning of the request handler.
pkg/logproto/compat.go
Outdated
|
|
||
| return fmt.Sprintf("%d/%s", chunksHash, sb.String()) | ||
| // TODO(salvacorts): plan.String() will return the whole query. This is not optimal since we are only interested in the filter expressions. | ||
| return fmt.Sprintf("%d/%s", chunksHash, plan) |
There was a problem hiding this comment.
Do we have a limit of the cache key length? Plan could be up to 2k characters, right?
Would a query hash be sufficient, since we have the combination with the chunksHash?
There was a problem hiding this comment.
Done, changed to use plan hash
| // TODO(chaudum): Take the chunks from the index querier's GetChunks() | ||
| // response and send them to the bloom gateway along with the filter | ||
| // expression that we got from the request object. | ||
| // The bloom gateway returns the list of matching ChunkRefs. |
There was a problem hiding this comment.
This comment can be removed.
What this PR does / why we need it:
Follow-up for #12035.
Before this PR, the
FilterChunkRefRequestwould contain a list of filters ([]syntax.LineFilter). In order to build a proper bloom test plan with the filters, we would need to have the filters as[]syntax. LineFilterExprbut:syntax. LineFilterExprwhich is a tree-like structure which might get cumbersome to marshal.[]syntax. LineFilterExpr.Instead, we are passing the query plan with
FilterChunkRefRequest. Later on, in the bloom-gateway we extract the filters ([]syntax. LineFilterExpr) from the plan.Special notes for your reviewer:
This PR doesn't modify the bloom test logic. We still extract a slice of
syntax.LineFilter(matching filters only. i.e. no regexes not Not Equal matches) out of the plan (regardless of the Or operations) and build ngrams for the matches.On a followup PR, we will wire the changes from this PR to the new tests from #12035.
Checklist
CONTRIBUTING.mdguide (required)CHANGELOG.mdupdatedadd-to-release-noteslabeldocs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR