chore: Aggregation groupings for by() and without() #19928
Conversation
spiridonov
changed the title
…spiridonov-more-funcs
|
|
||
| // One of the parsed columns | ||
| case ident.ColumnType() == types.ColumnTypeParsed: | ||
| case ident.ColumnType() == types.ColumnTypeParsed || (ident.ColumnType() == types.ColumnTypeGenerated && |
There was a problem hiding this comment.
is this added to handle error columns that are of type Generated?
| fmt.Sprintf(`count_over_time(%s[%s])`, selector, rangeInterval), | ||
| fmt.Sprintf(`count_over_time(%s | detected_level=~"error|warn" [%s])`, selector, rangeInterval), | ||
| fmt.Sprintf(`count_over_time(%s |= "level" [%s])`, selector, rangeInterval), | ||
| //fmt.Sprintf(`avg_over_time(%s | json | unwrap rows_affected [%s])`, selector, rangeInterval), |
There was a problem hiding this comment.
i think we should uncomment these if tests are passing or do that in a follow-up after fixing the correctness issues
|
|
||
| const ( | ||
| GroupingModeInvalid GroupingMode = iota | ||
| GroupingModeByEmptySet // Grouping by empty label set: <operation> by () (<expr>) |
There was a problem hiding this comment.
do we need 4 of these? GroupingModeByEmptySet is the same as GroupingModeByLabelSet without any groupings right
This can also be a flag. without=(true|false)
|
|
||
| // Aggregation operation to perform over the underlying range vector. | ||
| AggregateVectorOp operation = 2; | ||
| AggregateVectorOp operation = 3; |
There was a problem hiding this comment.
nit: no need to reorder this entry
| panic("len(labels) != len(labelValues)") | ||
| } | ||
|
|
||
| for _, label := range labels { |
There was a problem hiding this comment.
this can be expensive as we run it for each row. can we check the benchmarks for a metric test with this change?
|
|
||
| t.Run("basic SUM aggregation with record building", func(t *testing.T) { | ||
| agg := newAggregator(groupBy, 10, aggregationOperationSum) | ||
| agg := newAggregator(10, aggregationOperationSum) |
There was a problem hiding this comment.
can we add more aggregation tests that call Add() with different labels as the existing ones assume the same labels each time?
| // rangeAggregationOperations holds the mapping of range aggregation types to operations for an aggregator. | ||
| rangeAggregationOperations = map[types.RangeAggregationType]aggregationOperation{ | ||
| types.RangeAggregationTypeSum: aggregationOperationSum, | ||
| types.RangeAggregationTypeCount: aggregationOperationCount, |
There was a problem hiding this comment.
any reason why these are removed?
|
|
||
| for _, w := range windows { | ||
| r.aggregator.Add(w.end, value, labelValues) | ||
| r.aggregator.Add(w.end, value, labels, labelValues) |
There was a problem hiding this comment.
as we know the labels of a record, can we give a hint to the aggregator once per record to update its label set instead of doing it once per row?
2 participants
What this PR does / why we need it:
This PR introduces proper groupings (a list of columns and a mode by/without). Previously it was represented only a s list of columns and
withoutwas not supported at all.aggregatoris changed to aggregate by an arbitrary variable list of labels. The resulted record will have a union of all columns seen during aggregation.without ()we have to read all columns from data objects and nothing can be pushed down.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