chore(engine): adds AST conversion and result builders for metric queries #18166
Conversation
|
|
||
| func buildPlanForSampleQuery(e syntax.SampleExpr, params logql.Params) (*Builder, error) { | ||
| if params.Step() > 0 { | ||
| return nil, fmt.Errorf("only instant metric queries are supported") |
There was a problem hiding this comment.
Should this return errUnimplemented? Afaik, we check in the http handler for that error to decide whether we need to fall back to the old engine.
There was a problem hiding this comment.
added in 8997274
i think all error branches need to either log more details about the specific operation that is not supported or return error with more context. I can take that in a follow-up
| @@ -27,7 +27,7 @@ var ( | |||
| // providing this information (e.g. pg_catalog, ...) whereas in Loki there | |||
| // is the Metastore. | |||
| type Catalog interface { | |||
| ResolveDataObj(Expression) ([]DataObjLocation, [][]int64, error) | |||
| ResolveDataObj(Expression, time.Duration) ([]DataObjLocation, [][]int64, error) | |||
There was a problem hiding this comment.
This feels like a weird change to the Catalog API. Either we should pass no time information, or the full time information - otherwise it will get confusing.
There was a problem hiding this comment.
i agree :)
- we could add $range to the catalog state when scanning the AST. But as you mentioned in the other comment each branch of a binary can have different ranges, so this does not feel like the right place.
- I like the idea of passing both start and end time to the ResolveDataObj API.
2nd approach would require either the physical planner state or MakeTable logical plan node to hold start and end time. Putting this information in planner state sounds like a good start to me as this will get used in both MakeTable and TimeRange predicate creation. wdyt?
|
|
||
| "github.com/grafana/loki/v3/pkg/engine/planner/logical" | ||
| ) | ||
|
|
||
| // Internal state of the planner | ||
| type state struct { | ||
| direction SortOrder | ||
| direction SortOrder | ||
| rangeInterval time.Duration // for queries with [$range] |
There was a problem hiding this comment.
How do we deal with binop queries that have different ranges on their left and right branches? I guess it would work out of the box, because the state will refer to the most recent visited branch.
There was a problem hiding this comment.
thinking about it now, this approach might not work with the current instruction traversing (it looks like BFS?)
if it were DFS, the $range from an aggregation would get correctly picked by its child MakeTable
There was a problem hiding this comment.
should work i think as we are doing DFS. this could be error prone though, should i add a TODO with a comment so we can revisit when we add more operations that make use of the planner state?
| case types.ColumnNameGeneratedValue: | ||
| if col.IsNull(i) { | ||
| return promql.Sample{}, false | ||
| } | ||
|
|
||
| sample.F = float64(col.(*array.Int64).Value(i)) |
There was a problem hiding this comment.
Should we enforce that a generated column can only be of type float when creating the column?
|
|
||
| func (b *vectorResultBuilder) collectRow(rec arrow.Record, i int) (promql.Sample, bool) { | ||
| var sample promql.Sample | ||
| lbls := labels.NewBuilder(labels.EmptyLabels()) |
There was a problem hiding this comment.
Can the builder be re-used?
What this PR does / why we need it:
sum by (group) count_over_time{...[$range]}promql.Vectorfor instant metric queries[$range]Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
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