Maybe a comment about why there are sorted.

Could you do a

defer cache[Req{}] = append(cache[Req{}], outSeries...)

here instead of all the individual cache steps?

very nice, I like it

defer args are evaluated at call time, so this won't work as expected.

edit: looks like i'm wrong: https://play.golang.org/p/t8RZ1v8fLXT

You forgot to call foo() in your snippet.
see https://play.golang.org/p/Qh4rp1p2q73

I think this violates https://github.com/grafana/metrictank/blob/master/expr/NOTES#L35

I think this violates https://github.com/grafana/metrictank/blob/master/expr/NOTES#L35

I think this violates https://github.com/grafana/metrictank/blob/master/expr/NOTES#L35

fixed all and added a test

Could you use something like

No, because the original series does get modified by metrictank (a "name" tag gets added). Makes sense to compare relevant values only.

Scratch that. the real problem was that reflect.DeepEqual(math.NaN(), math.NaN()) == false, which is not the case when comparing series

maybe just

originalSeries[i] = serie
originalSeries[i].Datapoints = getCopy(serie.Datapoints)

Gets all the things like tags etc.

Nit, but 'Mimic' the common modern spelling

the removal of cutoff here is this safe?

Yes. Before it assumed that series arguments can only be non-optional arguments and in the beginning. i.e function(serie, int, string, opt=string).

With my change the following is possible:
function(serie, int, serie, opt=serie)

Like before, this part only consumes series, and just skips over everything else.

looks good :) the argument consumption/iterating code is probably my least favorite code of MT. FWIW.
if you can handle this, you can handle anything else 👯‍♂️

seems like it's fairly easy to trigger this by specifying a serieslist pattern that doesn't match any series.
maybe in that case we can provide a better error message? (basically above at if len(totals) == 0 { maybe directly return an error ? what does graphite do in this case?

I'm not sure what you mean here. This error message gets triggered if they pass in a number. In the case where they pass in a nodes argument, only a series or nothing should be passed in for the total argument. First branch is triggered if neither is passed in. Second branch is triggered if a series is passed in. Which leaves the case where a number is passed in (i.e math.IsNaN(s.totalFloat) = false)

Graphite throws that exact error message is that case.

if you specify a serieslist pattern that doesn't match any series, then I believe this will happen:

        if s.totalSeries != nil {
                totals, err = s.totalSeries.Exec(cache)
                if err != nil {
                        return nil, err
                }
                if len(totals) == 0 {
                        totals = nil <---- this right here
                }
        }

FWIW, Graphite crashes if you do that.

Let me see if I can handle it in some clean way

Ok, so this was just unnecessary:

if len(totals) == 0 {
            totals = nil
}

So I removed it. Now if a series returns empty, it does not assume that there were no arguments

this function is rather complex. can we split it up in smaller functions?

series, totals []models.Series. also the other function

this API is a bit strange. would it not make more sense to return a new series that is a deep copy?
even though that is slightly more work, seems worth it.

in fact, maybe add a Copy() method to the serie type in the models package

also looking at sumSeries, the copying/newly-allocating seems a bit too eager.
we should only need to allocate point slices:

1. for the totals slice: when we need to sum up values and we need a place to store the totals (not when totals results in a single series, then we can just read from it)
2. for each output series.(if it is different from the input slice)

The problem with # 1 is that later I modify the total series in some cases such as this (line 99):

// No series for total series
if _, ok := metaSeries[key]; !ok {
	serie2 := totalSeries[key]
	serie2.QueryPatt = fmt.Sprintf("asPercent(MISSING,%s)", serie2.QueryPatt)
	serie2.Target = fmt.Sprintf("asPercent(MISSING,%s)", serie2.Target)
	serie2.Tags = map[string]string{"name": serie2.Target}
	for i := range serie2.Datapoints {
		serie2.Datapoints[i].Val = math.NaN()
	}
	outSeries = append(outSeries, serie2)
	continue
}

I guess I could make a copy right there, not sure which one would be better.

I'll be mostly afk until Thursday.
Please read expr/NOTES carefully if you have not already done so. All this complexity is to make sure we never overwrite data in the memory AggMetric, chunk cache etc.
Some of these fields are by value so harmless but the tags map is interesting as well as it might open an avenue to modify the tags in the MemoryIdx. Not sure if we currently take that into account everywhere or maybe we already have a provision for that. On phone so can't check right now

Ok, well I added a Series.Copy(emptyDatapoints []schema.Point) function (the argument is there so that pointSlicePool can be used if needed). And I only copy the series if I modify values that shouldn't be modified.

Hopefully that's sufficient and what you were looking for 😃

shouldn't out2 be the same as in the previous function? because both cases have asPercent(d,a)

No, because when totals is a seriesList with len(totals) == len(series), the series first get sorted by tag before getting matched. So, in this test case it would be asPercent(d,c) and asPercent(b,a). This behavior is same in Graphite

isn't totalFloat pretty much guaranteed to be NaN here?

No, because totalFloat is still a valid option here. (the else part of the if statement right before this)

we may want to always just assume this branch is taken and just write this code line directly without the if/else.
that way if we ever have a bug where len(totalSerie.Datapoints) != len(serie.Datapoints) we can troubleshoot it instead of trying to hide such error case and returning incorrect data.

this is more of a check of whether there is a totalSeries at all. I'll change it to > 0, that way we get an index out of range exception if there's a bug.

lots of duplication here wrt the similar code block further down.
can't we do all the if/else stuff here to just set up the right totals and series variables,
and then do the same processing at the end irrespective of which scenario it was?

• we compute totals even for keys we won't need (e.g. keys not in the input series)
• for each missing case we repeatedly get a slice and fill it with NaN's. we could reuse the same slice in this case. my suggestion would be declare a var nones []schema.Point. then whenever you need it, if it's nil, instantiate it. if it's not, just reuse it.

I don't think reusing a nones slice would play nicely with the pointSlicePool, would it?

oh right, because at the end the same slice would get added to the pool multiple times, which is bad. so nevermind that then.

On phone now
best would be to just add the slice to the cache thing once I think (that way we can reuse them)