Exp2 exp3 exp refactor

api/graphite.go

@@ -4,7 +4,6 @@ import (
 	"errors"
 	"math"
 	"net/http"
-	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -204,7 +203,6 @@ func (s *Server) renderMetrics(ctx *middleware.Context, request models.GraphiteR
 		ctx.Error(http.StatusBadRequest, err.Error())
 		return
 	}
-	sort.Sort(models.SeriesByTarget(out))
 
 	switch request.Format {
 	case "msgp":

docs/graphite.md

@@ -17,3 +17,4 @@ averageSeries(seriesLists) series                    | avg          | Stable
 consolidateBy(seriesList, func) seriesList           |              | Stable
 movingAverage(seriesLists, windowSize) seriesList    |              | Unstable
 sumSeries(seriesLists) series                        | sum          | Stable
+transformNull(seriesList, default=0) seriesList      |              | Stable

expr/LICENSE

@@ -0,0 +1,26 @@
+Regarding type expr, much of parse.go, parse_test.go:
+
+Copyright (c) 2017 Grafana Labs
+Copyright (c) 2014,2015 Damian Gryski <damian@gryski.com>
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are met:
+
+* Redistributions of source code must retain the above copyright notice,
+this list of conditions and the following disclaimer.
+
+* Redistributions in binary form must reproduce the above copyright notice,
+this list of conditions and the following disclaimer in the documentation
+and/or other materials provided with the distribution.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

expr/NOTES

@@ -1,4 +1,23 @@
-regarding when to allocate models.Series (or more precisely, their []schema.Point attribute), there's 2 main choices:
+## future code quality improvements
+
+The code is a bit ugly in some places, there's a few items I'm not happy with and hope to change down the road, ideally after building out the feature more and getting a better understanding of requirements. In particular:
+
+* do all validation in planner, nothing should be needed in individual functions Init invocations
+  though often validation and storing a value are intertwined. should ideally feed function post-validation value (e.g. movingAverage windowsize)
+* in func Exec, uniform access to optional args, whether specified via kwarg or position.
+* Init and NeedRange could be 1 function, though not sure if they should
+* we instantiate and init functions twice (during creation of plan and running it). could do this only once if ...
+function invocations could be attached to expressions (when creating the plan). when executing them, we then
+also don't need to pass args anymore (which is currently redundant since they already got them via Init)
+* in general, while constructing the plan, we should probably build a different datastructure more tailored towards execution, currently we
+  stick to the parse-time expr types in the planner and these concepts also make their way into the function implementations, which gets a bit dirty at times.
+
+movingAverage accepts arg as both int or string, how to handle properly?
+
+
+## regarding when to allocate models.Series (or more precisely, their []schema.Point attribute)
+
+there's 2 main choices:
 
 1) copy-on-write:
 - each function does not modify data in their inputs, they allocate new slices (or better: get from pool) etc to save output computations
@@ -29,3 +48,6 @@ note that individual processing functions only request slices from the pool, the
 e.g. an avg of 3 series will create 1 new series (from pool), but won't put the 3 inputs back in the pool, because
 another processing step may require the same input data.
 
+function implementations:
+* must not modify existing slices
+* should use the pool to get new slices in which to store their new/modified data. and add the new slice into the cache so it can later be cleaned

expr/data_test.go

@@ -33,6 +33,16 @@ var c = []schema.Point{
 	{Val: 4, Ts: 60},
 }
 
+// emulate an 8 bit counter
+var d = []schema.Point{
+	{Val: 0, Ts: 10},
+	{Val: 33, Ts: 20},
+	{Val: 199, Ts: 30},
+	{Val: 29, Ts: 40}, // overflowed
+	{Val: 80, Ts: 50},
+	{Val: 250, Ts: 60},
+}
+
 var sumab = []schema.Point{
 	{Val: 0, Ts: 10},
 	{Val: math.MaxFloat64, Ts: 20},

expr/expr.go

@@ -10,17 +10,21 @@ type exprType int
 
 // the following types let the parser express the type it parsed from the input targets
 const (
-	etName   exprType = iota // a string without quotes, e.g. metric.name, metric.*.query.patt* or special values like True or None which some functions expect
+	etName   exprType = iota // a string without quotes, e.g. metric.name, metric.*.query.patt* or special values like None which some functions expect
+	etBool                   // True or False
 	etFunc                   // a function call like movingAverage(foo, bar)
-	etConst                  // any number, parsed as a float64 value
+	etInt                    // any number with no decimal numbers, parsed as a float64 value
+	etFloat                  // any number with decimals, parsed as a float64 value
 	etString                 // anything that was between '' or ""
 )
 
 // expr represents a parsed expression
 type expr struct {
 	etype     exprType
-	float     float64          // for etConst
-	str       string           // for etName, etFunc (func name), etString and etConst (unparsed input value)
+	float     float64          // for etFloat
+	int       int64            // for etInt
+	str       string           // for etName, etFunc (func name), etString, etBool, etInt and etFloat (unparsed input value)
+	bool      bool             // for etBool
 	args      []*expr          // for etFunc: positional args which itself are expressions
 	namedArgs map[string]*expr // for etFunc: named args which itself are expressions
 	argsStr   string           // for etFunc: literal string of how all the args were specified
@@ -40,10 +44,199 @@ func (e expr) Print(indent int) string {
 			args += strings.Repeat(" ", indent+2) + k + "=" + v.Print(0) + ",\n"
 		}
 		return fmt.Sprintf("%sexpr-func %s(\n%s%s)", space, e.str, args, space)
-	case etConst:
-		return fmt.Sprintf("%sexpr-const %v", space, e.float)
+	case etFloat:
+		return fmt.Sprintf("%sexpr-float %v", space, e.float)
+	case etInt:
+		return fmt.Sprintf("%sexpr-int %v", space, e.int)
 	case etString:
 		return fmt.Sprintf("%sexpr-string %q", space, e.str)
 	}
 	return "HUH-SHOULD-NEVER-HAPPEN"
 }
+
+// consumeBasicArg verifies that the argument at given pos matches the expected arg
+// it's up to the caller to assure that given pos is valid before calling.
+// if arg allows for multiple arguments, pos is advanced to cover all accepted arguments.
+// if the arg is a "basic" arg (meaning not a series, seriesList or seriesLists) the
+// appropriate value(s) will be assigned to exp.val
+// for non-basic args, see consumeSeriesArg which should be called after deducing the required from/to.
+// the returned pos is always the index where the next argument should be.
+func (e expr) consumeBasicArg(pos int, exp Arg) (int, error) {
+	got := e.args[pos]
+	switch v := exp.(type) {
+	case ArgSeries, ArgSeriesList:
+		if got.etype != etName && got.etype != etFunc {
+			return 0, ErrBadArgumentStr{"func or name", string(got.etype)}
+		}
+	case ArgSeriesLists:
+		if got.etype != etName && got.etype != etFunc {
+			return 0, ErrBadArgumentStr{"func or name", string(got.etype)}
+		}
+		// special case! consume all subsequent args (if any) in args that will also yield a seriesList
+		for len(e.args) > pos+1 && (e.args[pos+1].etype == etName || e.args[pos+1].etype == etFunc) {
+			pos += 1
+		}
+	case ArgInt:
+		if got.etype != etInt {
+			return 0, ErrBadArgumentStr{"int", string(got.etype)}
+		}
+		for _, va := range v.validator {
+			if err := va(got); err != nil {
+				return 0, fmt.Errorf("%s: %s", v.key, err.Error())
+			}
+		}
+		*v.val = got.int
+	case ArgInts:
+		if got.etype != etInt {
+			return 0, ErrBadArgumentStr{"int", string(got.etype)}
+		}
+		*v.val = append(*v.val, got.int)
+		// special case! consume all subsequent args (if any) in args that will also yield an integer
+		for len(e.args) > pos+1 && e.args[pos+1].etype == etInt {
+			pos += 1
+			for _, va := range v.validator {
+				if err := va(e.args[pos]); err != nil {
+					return 0, fmt.Errorf("%s: %s", v.key, err.Error())
+				}
+			}
+			*v.val = append(*v.val, e.args[pos].int)
+		}
+	case ArgFloat:
+		// integer is also a valid float, just happened to have no decimals
+		if got.etype != etFloat && got.etype != etInt {
+			return 0, ErrBadArgumentStr{"float", string(got.etype)}
+		}
+		for _, va := range v.validator {
+			if err := va(got); err != nil {
+				return 0, fmt.Errorf("%s: %s", v.key, err.Error())
+			}
+		}
+		if got.etype == etInt {
+			*v.val = float64(got.int)
+		} else {
+			*v.val = got.float
+		}
+	case ArgString:
+		if got.etype != etString {
+			return 0, ErrBadArgumentStr{"string", string(got.etype)}
+		}
+		for _, va := range v.validator {
+			if err := va(got); err != nil {
+				return 0, fmt.Errorf("%s: %s", v.key, err.Error())
+			}
+		}
+		*v.val = got.str
+	case ArgBool:
+		if got.etype != etBool {
+			return 0, ErrBadArgumentStr{"string", string(got.etype)}
+		}
+		*v.val = got.bool
+	default:
+		return 0, fmt.Errorf("unsupported type %T for consumeBasicArg", exp)
+	}
+	pos += 1
+	return pos, nil
+}
+
+// consumeSeriesArg verifies that the argument at given pos matches the expected arg
+// it's up to the caller to assure that given pos is valid before calling.
+// if arg allows for multiple arguments, pos is advanced to cover all accepted arguments.
+// if the arg is a "basic", no value is saved (it's up to consumeBasicArg to do that)
+// but for non-basic args (meaning a series, seriesList or seriesLists) the
+// appropriate value(s) will be assigned to exp.val
+// the returned pos is always the index where the next argument should be.
+func (e expr) consumeSeriesArg(pos int, exp Arg, from, to uint32, stable bool, reqs []Req) (int, []Req, error) {
+	got := e.args[pos]
+	var err error
+	var fn GraphiteFunc
+	switch v := exp.(type) {
+	case ArgSeries:
+		if got.etype != etName && got.etype != etFunc {
+			return 0, nil, ErrBadArgumentStr{"func or name", string(got.etype)}
+		}
+		fn, reqs, err = newplan(got, from, to, stable, reqs)
+		if err != nil {
+			return 0, nil, err
+		}
+		*v.val = fn
+	case ArgSeriesList:
+		if got.etype != etName && got.etype != etFunc {
+			return 0, nil, ErrBadArgumentStr{"func or name", string(got.etype)}
+		}
+		fn, reqs, err = newplan(got, from, to, stable, reqs)
+		if err != nil {
+			return 0, nil, err
+		}
+		*v.val = fn
+	case ArgSeriesLists:
+		if got.etype != etName && got.etype != etFunc {
+			return 0, nil, ErrBadArgumentStr{"func or name", string(got.etype)}
+		}
+		fn, reqs, err = newplan(got, from, to, stable, reqs)
+		if err != nil {
+			return 0, nil, err
+		}
+		*v.val = append(*v.val, fn)
+		// special case! consume all subsequent args (if any) in args that will also yield a seriesList
+		for len(e.args) > pos+1 && (e.args[pos+1].etype == etName || e.args[pos+1].etype == etFunc) {
+			pos += 1
+			fn, reqs, err = newplan(e.args[pos], from, to, stable, reqs)
+			if err != nil {
+				return 0, nil, err
+			}
+			*v.val = append(*v.val, fn)
+		}
+	default:
+		return 0, nil, fmt.Errorf("unsupported type %T for consumeSeriesArg", exp)
+	}
+	pos += 1
+	return pos, reqs, nil
+}
+
+// consumeKwarg consumes the kwarg (by key k) and verifies it
+// if the specified argument is valid, it is saved in exp.val
+// where exp is the arg specified by the function that has the given key
+func (e expr) consumeKwarg(key string, optArgs []Arg) error {
+	var found bool
+	var exp Arg
+	for _, exp = range optArgs {
+		if exp.Key() == key {
+			found = true
+			break
+		}
+	}
+	if !found {
+		return ErrUnknownKwarg{key}
+	}
+	got := e.namedArgs[key]
+	switch v := exp.(type) {
+	case ArgInt:
+		if got.etype != etInt {
+			return ErrBadKwarg{key, exp, got.etype}
+		}
+		*v.val = got.int
+	case ArgFloat:
+		switch got.etype {
+		case etInt:
+			// integer is also a valid float, just happened to have no decimals
+			*v.val = float64(got.int)
+		case etFloat:
+			*v.val = got.float
+		default:
+			return ErrBadKwarg{key, exp, got.etype}
+		}
+	case ArgString:
+		if got.etype != etString {
+			return ErrBadKwarg{key, exp, got.etype}
+		}
+		*v.val = got.str
+	case ArgBool:
+		if got.etype != etBool {
+			return ErrBadKwarg{key, exp, got.etype}
+		}
+		*v.val = got.bool
+	default:
+		return fmt.Errorf("unsupported type %T for consumeKwarg", exp)
+	}
+	return nil
+}

expr/func_alias.go

@@ -1,39 +1,36 @@
 package expr
 
 import (
-	"reflect"
-
 	"github.com/raintank/metrictank/api/models"
 )
 
 type FuncAlias struct {
+	in    GraphiteFunc
+	alias string
 }
 
-func NewAlias() Func {
-	return FuncAlias{}
-}
-
-func (s FuncAlias) Signature() ([]argType, []argType) {
-	return []argType{seriesList, str}, []argType{seriesList}
+func NewAlias() GraphiteFunc {
+	return &FuncAlias{}
 }
 
-func (s FuncAlias) Init(args []*expr) error {
-	return nil
+func (s *FuncAlias) Signature() ([]Arg, []Arg) {
+	return []Arg{
+		ArgSeriesList{val: &s.in},
+		ArgString{val: &s.alias},
+	}, []Arg{ArgSeriesList{}}
 }
 
-func (s FuncAlias) NeedRange(from, to uint32) (uint32, uint32) {
+func (s *FuncAlias) NeedRange(from, to uint32) (uint32, uint32) {
 	return from, to
 }
 
-func (s FuncAlias) Exec(cache map[Req][]models.Series, in ...interface{}) ([]interface{}, error) {
-	series, ok := in[0].([]models.Series)
-	if !ok {
-		return nil, ErrBadArgument{reflect.TypeOf([]models.Series{}), reflect.TypeOf(in[0])}
+func (s *FuncAlias) Exec(cache map[Req][]models.Series) ([]models.Series, error) {
+	series, err := s.in.Exec(cache)
+	if err != nil {
+		return nil, err
 	}
-	var out []interface{}
-	for _, serie := range series {
-		serie.Target = in[1].(string)
-		out = append(out, serie)
+	for i := range series {
+		series[i].Target = s.alias
 	}
-	return out, nil
+	return series, nil
 }