[query] Fix Graphite timeShift series renaming (#3643)

src/query/graphite/native/alias_functions_test.go

@@ -24,11 +24,14 @@ import (
 	"testing"
 	"time"
 
-	"github.com/m3db/m3/src/query/graphite/common"
-	"github.com/m3db/m3/src/query/graphite/ts"
-
+	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/m3db/m3/src/query/graphite/common"
+	"github.com/m3db/m3/src/query/graphite/storage"
+	"github.com/m3db/m3/src/query/graphite/ts"
+	xgomock "github.com/m3db/m3/src/x/test"
 )
 
 func TestAlias(t *testing.T) {
@@ -250,3 +253,29 @@ func TestAliasByNodeWitCallSubExpressions(t *testing.T) {
 	assert.Equal(t, "foo01", results.Values[0].Name())
 	assert.Equal(t, "foo02", results.Values[1].Name())
 }
+
+// TestExecuteAliasByNodeAndTimeShift tests that the output of timeshift properly
+// quotes the time shift arg so that it appears as a string and can be used to find
+// the inner path expression without failing compilation when aliasByNode finds the
+// first path element.
+func TestExecuteAliasByNodeAndTimeShift(t *testing.T) {
+	ctrl := xgomock.NewController(t)
+	defer ctrl.Finish()
+
+	store := storage.NewMockStorage(ctrl)
+
+	engine := NewEngine(store, CompileOptions{})
+
+	ctx := common.NewContext(common.ContextOptions{Start: time.Now().Add(-1 * time.Hour), End: time.Now(), Engine: engine})
+
+	stepSize := int((10 * time.Minute) / time.Millisecond)
+	store.EXPECT().FetchByQuery(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
+		buildTestSeriesFn(stepSize, "foo.bar.q.zed", "foo.bar.g.zed",
+			"foo.bar.x.zed"))
+
+	expr, err := engine.Compile("aliasByNode(timeShift(foo.bar.*.zed,'-7d'), 0)")
+	require.NoError(t, err)
+
+	_, err = expr.Execute(ctx)
+	require.NoError(t, err)
+}

src/query/graphite/native/builtin_functions.go

@@ -382,7 +382,7 @@ func timeShift(
 		output := make([]*ts.Series, input.Len())
 		for i, in := range input.Values {
 			// NB(jayp): opposite direction
-			output[i] = in.Shift(-1 * shift).RenamedTo(fmt.Sprintf("timeShift(%s, %s)", in.Name(), timeShiftS))
+			output[i] = in.Shift(-1 * shift).RenamedTo(fmt.Sprintf("timeShift(%s,%q)", in.Name(), timeShiftS))
 		}
 		input.Values = output
 		return input, nil

src/query/graphite/native/builtin_functions_test.go

@@ -3937,15 +3937,15 @@ func TestMovingAverage(t *testing.T) {
 	defer func() { _ = ctx.Close() }()
 
 	stepSize := 60000
-	target := `movingAverage(timeShift(foo.bar.g.zed, '-1d'), '1min', 0.7)`
+	target := `movingAverage(timeShift(foo.bar.g.zed,'-1d'), '1min', 0.7)`
 	store.EXPECT().FetchByQuery(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
 		buildTestSeriesFn(stepSize, "foo.bar.g.zed")).AnyTimes()
 	expr, err := engine.Compile(target)
 	require.NoError(t, err)
 	res, err := expr.Execute(ctx)
 	require.NoError(t, err)
 	expected := common.TestSeries{
-		Name: `movingAverage(timeShift(foo.bar.g.zed, -1d),"1min")`,
+		Name: `movingAverage(timeShift(foo.bar.g.zed,"-1d"),"1min")`,
 		Data: []float64{1, 1},
 	}
 	common.CompareOutputsAndExpected(t, stepSize, startTime,
@@ -3966,15 +3966,15 @@ func TestMovingWindow(t *testing.T) {
 	defer func() { _ = ctx.Close() }()
 
 	stepSize := 60000
-	target := `movingWindow(timeShift(foo.bar.g.zed, '-1d'), '1min', 'avg', 0.7)`
+	target := `movingWindow(timeShift(foo.bar.g.zed,'-1d'), '1min', 'avg', 0.7)`
 	store.EXPECT().FetchByQuery(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
 		buildTestSeriesFn(stepSize, "foo.bar.g.zed")).AnyTimes()
 	expr, err := engine.Compile(target)
 	require.NoError(t, err)
 	res, err := expr.Execute(ctx)
 	require.NoError(t, err)
 	expected := common.TestSeries{
-		Name: `movingAverage(timeShift(foo.bar.g.zed, -1d),"1min")`,
+		Name: `movingAverage(timeShift(foo.bar.g.zed,"-1d"),"1min")`,
 		Data: []float64{1, 1},
 	}
 	common.CompareOutputsAndExpected(t, stepSize, startTime,
@@ -4334,7 +4334,7 @@ func TestTimeShift(t *testing.T) {
 	res, err := expr.Execute(ctx)
 	require.NoError(t, err)
 	expected := common.TestSeries{
-		Name: "timeShift(foo.bar.q.zed, -1min)",
+		Name: `timeShift(foo.bar.q.zed,"-1min")`,
 		Data: []float64{0.0, 0.0},
 	}
 	common.CompareOutputsAndExpected(t, stepSize, startTime,

src/query/graphite/native/compiler.go

@@ -284,13 +284,22 @@ func (c *compiler) compileArg(
 	}
 
 	if !arg.CompatibleWith(reflectType) {
-		return nil, false, c.errorf("invalid function call %s, arg %d: expected a %s, received '%s'",
-			fname, index, reflectType.Name(), arg)
+		return nil, false, c.errorf("invalid function call %s, arg %d: expected a %s, received a %s '%s'",
+			fname, index, reflectTypeName(reflectType), reflectTypeName(arg.Type()), arg)
 	}
 
 	return arg, false, nil
 }
 
+// reflectTypeName will dereference any pointer types to their base name
+// so that function call or fetch expression can be referenced by their name.
+func reflectTypeName(reflectType reflect.Type) string {
+	for reflectType.Kind() == reflect.Ptr {
+		reflectType = reflectType.Elem()
+	}
+	return reflectType.Name()
+}
+
 // convertTokenToArg converts the given token into the corresponding argument
 func (c *compiler) convertTokenToArg(token *lexer.Token, reflectType reflect.Type) (funcArg, error) {
 	switch token.TokenType() {

src/query/graphite/native/compiler_test.go

@@ -359,12 +359,12 @@ func TestCompileErrors(t *testing.T) {
 		{
 			"aliasByNode(10)",
 			"invalid expression 'aliasByNode(10)': invalid function call aliasByNode," +
-				" arg 0: expected a singlePathSpec, received '10'",
+				" arg 0: expected a singlePathSpec, received a float64 '10'",
 		},
 		{
 			"sortByName(hello())",
 			"invalid expression 'sortByName(hello())': invalid function call " +
-				"sortByName, arg 0: expected a singlePathSpec, received 'hello()'",
+				"sortByName, arg 0: expected a singlePathSpec, received a functionCall 'hello()'",
 		},
 		{
 			"aliasByNode()",
@@ -374,7 +374,7 @@ func TestCompileErrors(t *testing.T) {
 		{
 			"aliasByNode(foo.*.zed, 2, false)",
 			"invalid expression 'aliasByNode(foo.*.zed, 2, false)': invalid function call " +
-				"aliasByNode, arg 2: expected a int, received 'false'",
+				"aliasByNode, arg 2: expected a int, received a bool 'false'",
 		},
 		{
 			"aliasByNode(foo.*.bar,",
@@ -452,7 +452,7 @@ func TestCompileErrors(t *testing.T) {
 			"scale(servers.foobar*-qaz.quail.qux-qaz-qab.cpu.*, e)",
 			"invalid expression 'scale(servers.foobar*-qaz.quail.qux-qaz-qab.cpu.*, e)': " +
 				"invalid function call scale, " +
-				"arg 1: expected a float64, received 'fetch(e)'",
+				"arg 1: expected a float64, received a fetchExpression 'fetch(e)'",
 		},
 		{
 			"scale(servers.foobar*-qaz.quail.qux-qaz-qab.cpu.*, 1.2ee)",

src/query/graphite/native/expression.go

@@ -156,6 +156,10 @@ func (f *fetchExpression) Evaluate(ctx *common.Context) (reflect.Value, error) {
 	return reflect.ValueOf(timeseries), nil
 }
 
+func (f *fetchExpression) Type() reflect.Type {
+	return reflect.ValueOf(f).Type()
+}
+
 // CompatibleWith returns true if the reflected type is a time series or a generic interface.
 func (f *fetchExpression) CompatibleWith(reflectType reflect.Type) bool {
 	return reflectType == singlePathSpecType || reflectType == multiplePathSpecsType || reflectType == interfaceType

src/query/graphite/native/functions.go

@@ -507,6 +507,7 @@ func (f *Function) reflectCall(ctx *common.Context, args []reflect.Value) (refle
 type funcArg interface {
 	ASTNode
 	Evaluate(ctx *common.Context) (reflect.Value, error)
+	Type() reflect.Type
 	CompatibleWith(reflectType reflect.Type) bool
 }
 
@@ -522,6 +523,7 @@ func newFloat64Const(n float64) funcArg { return constFuncArg{value: reflect.Val
 func newIntConst(n int) funcArg         { return constFuncArg{value: reflect.ValueOf(n)} }
 
 func (c constFuncArg) Evaluate(ctx *common.Context) (reflect.Value, error) { return c.value, nil }
+func (c constFuncArg) Type() reflect.Type                                  { return c.value.Type() }
 func (c constFuncArg) CompatibleWith(reflectType reflect.Type) bool {
 	return c.value.Type() == reflectType || reflectType == interfaceType
 }
@@ -650,6 +652,10 @@ MaybeAdjustShiftLoop:
 	return ret[0], err
 }
 
+func (call *functionCall) Type() reflect.Type {
+	return reflect.ValueOf(call).Type()
+}
+
 // CompatibleWith checks whether the function call's return is compatible with the given reflection type
 func (call *functionCall) CompatibleWith(reflectType reflect.Type) bool {
 	if reflectType == interfaceType {