Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add comprehensive test and support for timestamp function #257

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 53 additions & 20 deletions execution/scan/vector_selector.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ type vectorSelector struct {
numShards int
}

type point struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this new struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well I made this just to have a good relation between timestamp and values in our data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what changes do we need now ?

t int64
v float64
fh *histogram.FloatHistogram
}

// NewVectorSelector creates operator which selects vector of series.
func NewVectorSelector(
pool *model.VectorPool,
Expand Down Expand Up @@ -106,35 +112,51 @@ func (o *vectorSelector) Next(ctx context.Context) ([]model.StepVector, error) {

vectors := o.vectorPool.GetVectorBatch()
ts := o.currentStep

for i := 0; i < len(o.scanners); i++ {
var (
series = o.scanners[i]
seriesTs = ts
series = o.scanners[i]
seriesTs = ts
lastSampleTs int64 // Added variable to store timestamp of the last sample in the lookback period.
)

for currStep := 0; currStep < o.numSteps && seriesTs <= o.maxt; currStep++ {
if len(vectors) <= currStep {
vectors = append(vectors, o.vectorPool.GetStepVector(seriesTs))
}
_, v, h, ok, err := selectPoint(series.samples, seriesTs, o.lookbackDelta, o.offset)

// Modify selectPoint call to retrieve timestamp of the last sample in the lookback period.
p, ok, err := selectPoint(series.samples, seriesTs, o.lookbackDelta, o.offset)
if err != nil {
return nil, err
}

if ok {
if h != nil {
vectors[currStep].AppendHistogram(o.vectorPool, series.signature, h)
if p.fh != nil {
vectors[currStep].AppendHistogram(o.vectorPool, series.signature, p.fh)
} else {
vectors[currStep].AppendSample(o.vectorPool, series.signature, v)
vectors[currStep].AppendSample(o.vectorPool, series.signature, p.v)
}

// Save the timestamp of the last sample in the lookback period.
lastSampleTs = p.t
}

seriesTs += o.step
}

// Use the saved timestamp to compute timestamp of last sample in lookback period.
if lastSampleTs > 0 {
vectors[len(vectors)-1].T = lastSampleTs
}
}

// For instant queries, set the step to a positive value
// so that the operator can terminate.
if o.step == 0 {
o.step = 1
}

o.currentStep += o.step * int64(o.numSteps)

return vectors, nil
Expand Down Expand Up @@ -165,35 +187,46 @@ 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I got the idea behind it !

refTime := ts - offset
var t int64
var v float64
var fh *histogram.FloatHistogram
var p point

returnTimestampAsValue := true // bool flag to indicate whether to return the timestamp as the point value.

valueType := it.Seek(refTime)
switch valueType {
case chunkenc.ValNone:
if it.Err() != nil {
return 0, 0, nil, false, it.Err()
return p, false, it.Err()
}
case chunkenc.ValFloatHistogram, chunkenc.ValHistogram:
t, fh = it.AtFloatHistogram()
t, fh := it.AtFloatHistogram()
p = point{t: t, fh: fh}
case chunkenc.ValFloat:
t, v = it.At()
t, v := it.At()
if returnTimestampAsValue {
p = point{t: t, v: float64(t)}

} else {
p = point{t: t, v: v}
}

default:
panic(errors.Newf("unknown value type %v", valueType))
}
if valueType == chunkenc.ValNone || t > refTime {

if valueType == chunkenc.ValNone || p.t > refTime {
var ok bool
t, v, _, fh, ok = it.PeekPrev()
if !ok || t < refTime-lookbackDelta {
return 0, 0, nil, false, nil
p.t, p.v, _, p.fh, ok = it.PeekPrev()
if !ok || p.t < refTime-lookbackDelta {
return p, false, nil
}
}
if value.IsStaleNaN(v) || (fh != nil && value.IsStaleNaN(fh.Sum)) {
return 0, 0, nil, false, nil

if value.IsStaleNaN(p.v) || (p.fh != nil && value.IsStaleNaN(p.fh.Sum)) {
return p, false, nil
}

return t, v, fh, true, nil
return p, true, nil
}