chore(dataobj): predicate pushdown metadata label filters #16846
Conversation
| Outer: | ||
| for i, stage := range pipelineExpr.MultiStages { | ||
| switch stage := stage.(type) { | ||
| case *syntax.LabelFmtExpr, *syntax.LineParserExpr, *syntax.LogfmtParserExpr, |
There was a problem hiding this comment.
Maybe pattern here too? That can add to the label set
There was a problem hiding this comment.
loki/pkg/logql/syntax/syntax.y.go
Lines 1371 to 1390 in 21bf5ee
LineParserExpr should cover it
ashwanthgoli
force-pushed
the
label-filter-predicate-pushdown
branch
from
21bf5ee to
7a4a514
Compare
github-actions
bot
added
the
type/docs
|
💻 Deploy preview available: |
There was a problem hiding this comment.
Nice start!
I had a few other comments that I deleted once I saw that we're not handling the stream label fallback case, and I think that's a more important discussion than any of my other suggestions.
It seems like trying to wire in predicate pushdown for metadata filters with the current querier code requires a fair amount of new complexity, and because it doesn't handle fallback, we also feel compelled to allow disabling it via the config since the fallback is required for correct query results.
I wonder if we should defer this to the new engine, where we can design it to more easily handle predicate pushdown of label expressions.
I don't know the answer here, and I'm going to trust your judgement. Here's some questions you can ask yourself that might help you decide:
-
What impact does this have on the benchmarks?
-
Given that impact, is the temporary additional complexity worth it?
-
Is it possible for the new engine to have simpler logic to pull off metadata predicate pushdown?
| "time" | ||
| ) | ||
|
|
||
| type ( | ||
| // Predicate is an expression used to filter entries in a data object. | ||
| Predicate interface { | ||
| isPredicate() | ||
| String() string |
There was a problem hiding this comment.
general question: do we want to make String() string a required method on every Predicate? Or may some of them just happen to implement String?
| Keep func(line []byte) bool | ||
| Desc string // Description of the filter for debugging. |
There was a problem hiding this comment.
nit: for consistency with the other function predicates, should Desc be before Keep here?
| var sb strings.Builder | ||
| sb.WriteString("Metadata(") | ||
| sb.WriteString(p.Key) | ||
| sb.WriteString("=") | ||
| sb.WriteString(p.Value) | ||
| sb.WriteString(")") | ||
| return sb.String() |
There was a problem hiding this comment.
Would
return fmt.Sprintf("Metadata(%s=%s)", p.Key, p.Value)here be simpler?
(Similar comment for other String methods that don't have if branches)
What this PR does / why we need it:
Follow-up to #16802, adds support to build dataobj reader predicates from label filters that refer to structured metadata columns. This is done by looking up the metadata columns of each dataobj section and applying filters that refer to any of these columns, so the predicates and expressions for readers that operate on different sections on an object need not be the same.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Not part of this pr, but some thoughts on applying stream filters: I think they can only be applied by looking at the labels of the matching streams for an object. For streams containing a label from the filter, we can look at the value to either discard streams that do not match or discard the label filter from expression to avoid redundant work if there is a match
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR