Add comprehensive test and support for timestamp function #257
Conversation
|
for adding Points slice we can update the Sample struct by adding a new Points field to it |
execution/function/functions.go
Outdated
| result := promql.Sample{} | ||
| for _, sample := range f.Samples { | ||
| if sample.H == nil { // only consider float samples | ||
| result.Points = append(result.Points, promql.Point{T: sample.T, V: float64(sample.T) / 1000}) |
There was a problem hiding this comment.
Hey, this cannot be implemented as a function i think, it will return the step timestamp for nested functions for example.
There was a problem hiding this comment.
yes you are right . I tried to push down the timestamp function into execution/scan/vector_selector.go as mentioned
| @@ -165,35 +187,38 @@ func (o *vectorSelector) loadSeries(ctx context.Context) error { | |||
| } | |||
|
|
|||
| // TODO(fpetkovski): Add max samples limit. | |||
| func selectPoint(it *storage.MemoizedSeriesIterator, ts, lookbackDelta, offset int64) (int64, float64, *histogram.FloatHistogram, bool, error) { | |||
| // To push down the timestamp function into this file and store the timestamp in the value for each series, you can modify the selectPoint function. | |||
| func selectPoint(it *storage.MemoizedSeriesIterator, ts, lookbackDelta, offset int64) (point, bool, error) { | |||
There was a problem hiding this comment.
My suggestion would be to roll back the changes and use a bool flag here to indicate whether to return the timestamp as the point value. Then for the case chunkenc.ValFloat case in the switch statement we can return t as the point value instead of v.
There was a problem hiding this comment.
Is it possible to return the timestamp as the point value incase chunkenc.ValFloatwithout the need to set a boolean flag for it? This would allow us to handle non-histogram data directly.
There was a problem hiding this comment.
Yes, the return types would remain as they are. The idea is to use a bool as an argument to the function in order to decide whether the chunkenc.ValFloat case would return t or v.
I guess we would need to do the same for histograms, based on this boolean we would either return the histogram itself or t as the float64 return value. If I'm not mistaken, the rest of the operator code would remain unchanged.
There was a problem hiding this comment.
Okay I got the idea behind it !
| @@ -49,6 +49,12 @@ type vectorSelector struct { | |||
| numShards int | |||
| } | |||
|
|
|||
| type point struct { | |||
There was a problem hiding this comment.
Do we still need this new struct?
There was a problem hiding this comment.
well I made this just to have a good relation between timestamp and values in our data.
There was a problem hiding this comment.
what changes do we need now ?
According to my understanding of the codebase, promql.Point is not defined in the Prometheus Go client library. Here's the definition of promql.Point that needs to be added in value.go file:
type Point struct {
T int64
V float64
}
also we can add a list of all the Points. It then loops over all the filtered samples and appends a promql.Point struct to its Points field with its timestamp and value (converted to seconds).
Resolve #206