Fix issue with nested binary operators counting as constant typed

CHANGELOG.md

@@ -29,6 +29,7 @@ kapacitor replay-live query -task cpu_alert -query 'SELECT usage_idle FROM teleg
 
 - [#540](https://github.com/influxdata/kapacitor/issues/540): Fixes bug with log level API endpoint.
 - [#521](https://github.com/influxdata/kapacitor/issues/521): EvalNode now honors groups.
+- [#561](https://github.com/influxdata/kapacitor/issues/561): Fixes bug when lambda expressions would return error about types with nested binary expressions.
 
 ## v0.13.1 [2016-05-13]
 

tick/lex.go

@@ -51,11 +51,18 @@ const (
 	//end mathematical operators
 	end_tok_operator_math
 
-	// begin comparison operators
-	begin_tok_operator_comp
+	// begin logical operators
+	begin_tok_operator_logic
 
 	TokenAnd
 	TokenOr
+
+	// end logical operators
+	end_tok_operator_logic
+
+	// begin comparison operators
+	begin_tok_operator_comp
+
 	TokenEqual
 	TokenNotEqual
 	TokenLess
@@ -182,6 +189,10 @@ func IsCompOperator(typ TokenType) bool {
 	return typ > begin_tok_operator_comp && typ < end_tok_operator_comp
 }
 
+func IsLogicalOperator(typ TokenType) bool {
+	return typ > begin_tok_operator_logic && typ < end_tok_operator_logic
+}
+
 // token represents a token or text string returned from the scanner.
 type token struct {
 	typ TokenType

tick/stateful/eval_binary_node.go

@@ -44,8 +44,8 @@ func (rc resultContainer) value() interface{} {
 // left side or right side
 type ErrSide struct {
 	error
-	IsLeftSide  bool
-	IsRightSide bool
+	IsLeft  bool
+	IsRight bool
 }
 
 // Evaluation functions
@@ -64,17 +64,20 @@ type EvalBinaryNode struct {
 
 	// Saving a cache version NodeEvaluator so we don't need to cast
 	// in every EvalBool call
-	leftSideEvaluator NodeEvaluator
-	leftSideType      ValueType
+	leftEvaluator NodeEvaluator
+	leftType      ValueType
 
-	rightSideEvaluator NodeEvaluator
-	rightSideType      ValueType
+	rightEvaluator NodeEvaluator
+	rightType      ValueType
 
 	// Return type
 	returnType ValueType
 }
 
 func NewEvalBinaryNode(node *tick.BinaryNode) (*EvalBinaryNode, error) {
+	if !tick.IsExprOperator(node.Operator) {
+		return nil, fmt.Errorf("unknown binary operator %v", node.Operator)
+	}
 	expression := &EvalBinaryNode{
 		operator:   node.Operator,
 		returnType: getConstantNodeType(node),
@@ -90,23 +93,26 @@ func NewEvalBinaryNode(node *tick.BinaryNode) (*EvalBinaryNode, error) {
 		return nil, fmt.Errorf("Failed to handle right node: %v", err)
 	}
 
-	expression.leftSideEvaluator = leftSideEvaluator
-	expression.rightSideEvaluator = rightSideEvaluator
+	expression.leftEvaluator = leftSideEvaluator
+	expression.rightEvaluator = rightSideEvaluator
 
 	if isDynamicNode(node.Left) || isDynamicNode(node.Right) {
 		expression.evaluationFn = expression.evaluateDynamicNode
 	} else {
-		expression.leftSideType = getConstantNodeType(node.Left)
-		expression.rightSideType = getConstantNodeType(node.Right)
+		expression.leftType = getConstantNodeType(node.Left)
+		expression.rightType = getConstantNodeType(node.Right)
 
-		expression.evaluationFn = expression.findEvaluationFn(expression.leftSideType, expression.rightSideType)
+		expression.evaluationFn = expression.lookupEvaluationFn()
+		if expression.evaluationFn == nil {
+			return nil, expression.determineError(nil, ExecutionState{})
+		}
 	}
 
 	return expression, nil
 }
 
 func (n *EvalBinaryNode) Type(scope ReadOnlyScope, executionState ExecutionState) (ValueType, error) {
-	return findNodeTypes(n.returnType, []NodeEvaluator{n.leftSideEvaluator, n.rightSideEvaluator}, scope, executionState)
+	return findNodeTypes(n.returnType, []NodeEvaluator{n.leftEvaluator, n.rightEvaluator}, scope, executionState)
 }
 
 func (n *EvalBinaryNode) EvalRegex(scope *tick.Scope, executionState ExecutionState) (*regexp.Regexp, error) {
@@ -178,11 +184,11 @@ func (e *EvalBinaryNode) EvalInt(scope *tick.Scope, executionState ExecutionStat
 
 func (e *EvalBinaryNode) eval(scope *tick.Scope, executionState ExecutionState) (resultContainer, *ErrSide) {
 	if e.evaluationFn == nil {
-		err := e.determineError(scope, executionState, e.operator, e.leftSideEvaluator, e.rightSideEvaluator)
+		err := e.determineError(scope, executionState)
 		return boolFalseResultContainer, &ErrSide{error: err}
 	}
 
-	evaluationResult, err := e.evaluationFn(scope, executionState, e.leftSideEvaluator, e.rightSideEvaluator)
+	evaluationResult, err := e.evaluationFn(scope, executionState, e.leftEvaluator, e.rightEvaluator)
 
 	// This case can in dynamic nodes,
 	// for example: RefNode("value") > NumberNode("float64")
@@ -192,18 +198,18 @@ func (e *EvalBinaryNode) eval(scope *tick.Scope, executionState ExecutionState)
 	if err != nil {
 		if typeGuardErr, isTypeGuardError := err.error.(ErrTypeGuardFailed); isTypeGuardError {
 			// Fix the type info, thanks to the type guard info
-			if err.IsLeftSide {
-				e.leftSideType = typeGuardErr.ActualType
+			if err.IsLeft {
+				e.leftType = typeGuardErr.ActualType
 			}
 
-			if err.IsRightSide {
-				e.rightSideType = typeGuardErr.ActualType
+			if err.IsRight {
+				e.rightType = typeGuardErr.ActualType
 			}
 
-			// re-find the evaluation fn
-			e.evaluationFn = e.findEvaluationFn(e.leftSideType, e.rightSideType)
+			// redefine the evaluation fn
+			e.evaluationFn = e.lookupEvaluationFn()
 
-			// recurse (so we handle nil evaluationFn, and etc)
+			// try again
 			return e.eval(scope, executionState)
 		}
 	}
@@ -214,8 +220,8 @@ func (e *EvalBinaryNode) eval(scope *tick.Scope, executionState ExecutionState)
 // evaluateDynamicNode fetches the value of the right and left node at evaluation time (aka "runtime")
 // and find the matching evaluation function for the givne types - this is where the "specialisation" happens.
 func (e *EvalBinaryNode) evaluateDynamicNode(scope *tick.Scope, executionState ExecutionState, left, right NodeEvaluator) (resultContainer, *ErrSide) {
-	var leftSideType ValueType
-	var rightSideType ValueType
+	var leftType ValueType
+	var rightType ValueType
 	var err error
 
 	// For getting the type we must pass new execution state, since the node can be stateful (like function call)
@@ -225,72 +231,69 @@ func (e *EvalBinaryNode) evaluateDynamicNode(scope *tick.Scope, executionState E
 	//  2. we evaluate the second time in "EvalBool"
 	typeExecutionState := CreateExecutionState()
 
-	if leftSideType, err = left.Type(scope, typeExecutionState); err != nil {
-		return emptyResultContainer, &ErrSide{error: err, IsLeftSide: true}
+	if leftType, err = left.Type(scope, typeExecutionState); err != nil {
+		return emptyResultContainer, &ErrSide{error: err, IsLeft: true}
 	}
 
-	if rightSideType, err = right.Type(scope, typeExecutionState); err != nil {
-		return emptyResultContainer, &ErrSide{error: err, IsRightSide: true}
+	if rightType, err = right.Type(scope, typeExecutionState); err != nil {
+		return emptyResultContainer, &ErrSide{error: err, IsRight: true}
 	}
 
-	e.leftSideType = leftSideType
-	e.rightSideType = rightSideType
+	e.leftType = leftType
+	e.rightType = rightType
 
-	e.evaluationFn = e.findEvaluationFn(e.leftSideType, e.rightSideType)
+	e.evaluationFn = e.lookupEvaluationFn()
 
 	return e.eval(scope, executionState)
 }
 
-func (e *EvalBinaryNode) determineError(scope *tick.Scope, executionState ExecutionState, operator tick.TokenType, left, right NodeEvaluator) error {
-	// Validate the evaluation parameters:
-	//  *) not support types like arrays
-	//  *) not comparison operator
-	//  *) invalid operator for the given type
-
-	typeExecutionState := CreateExecutionState()
-
-	leftValueType, err := left.Type(scope, typeExecutionState)
-	if err != nil {
-		return fmt.Errorf("Can't get the type of the left node: %s", err)
-	}
-	leftTypeName := leftValueType.String()
+// Return an understandable error which is most specific to the issue.
+func (e *EvalBinaryNode) determineError(scope *tick.Scope, executionState ExecutionState) error {
+	if scope != nil {
+		typeExecutionState := CreateExecutionState()
+		leftType, err := e.leftEvaluator.Type(scope, typeExecutionState)
+		if err != nil {
+			return fmt.Errorf("can't get the type of the left node: %s", err)
+		}
+		e.leftType = leftType
 
-	if leftValueType == InvalidType {
-		return errors.New("left value is invalid value type")
-	}
+		if leftType == InvalidType {
+			return errors.New("left value is invalid value type")
+		}
 
-	rightValueType, err := right.Type(scope, typeExecutionState)
-	if err != nil {
-		return fmt.Errorf("Can't get the type of the right node: %s", err)
-	}
-	rightTypeName := rightValueType.String()
+		rightType, err := e.rightEvaluator.Type(scope, typeExecutionState)
+		if err != nil {
+			return fmt.Errorf("can't get the type of the right node: %s", err)
+		}
+		e.rightType = rightType
 
-	if rightValueType == InvalidType {
-		return errors.New("right value is invalid value type")
+		if rightType == InvalidType {
+			return errors.New("right value is invalid value type")
+		}
 	}
 
-	err = isValidOperator(e.operator, leftValueType, rightValueType)
-	if err != nil {
-		return err
+	if !typeToBinaryOperators[e.leftType][e.operator] {
+		return fmt.Errorf("invalid %s operator %v for type %s", operatorKind(e.operator), e.operator, e.leftType)
+	} else if !typeToBinaryOperators[e.leftType][e.operator] {
+		return fmt.Errorf("invalid %s operator %v for type %s", operatorKind(e.operator), e.operator, e.rightType)
 	}
 
-	return fmt.Errorf("mismatched type to binary operator. got %s %v %s. see bool(), int(), float()", leftTypeName, e.operator, rightTypeName)
+	return fmt.Errorf("mismatched type to binary operator. got %s %v %s. see bool(), int(), float(), string()", e.leftType, e.operator, e.rightType)
 }
 
-func (e *EvalBinaryNode) findEvaluationFn(leftType, rightType ValueType) evaluationFn {
-	return evaluationFuncs[operationKey{operator: e.operator, leftType: leftType, rightType: rightType}]
+func (e *EvalBinaryNode) lookupEvaluationFn() evaluationFn {
+	return evaluationFuncs[operationKey{operator: e.operator, leftType: e.leftType, rightType: e.rightType}]
 }
 
-// Type to comparison operator - for comparison operator validation (see: isValidBinaryOperator)
-// The key is value type where the value is set of TokenType
 var typeToBinaryOperators = (func() map[ValueType]map[tick.TokenType]bool {
 	// This map is built at "runtime" because we don't to have tight coupling
 	// every time we had new "comparison operator" / "math operator" to update this map
 	// and the performance cost is neglibile for doing so.
 
-	result := make(map[ValueType]map[tick.TokenType]bool, 0)
+	result := make(map[ValueType]map[tick.TokenType]bool)
 
 	for opKey := range evaluationFuncs {
+		// Left
 		typeSet, exists := result[opKey.leftType]
 
 		if !exists {
@@ -299,43 +302,29 @@ var typeToBinaryOperators = (func() map[ValueType]map[tick.TokenType]bool {
 		}
 
 		typeSet[opKey.operator] = true
-		result[opKey.leftType] = typeSet
-	}
 
-	return result
-})()
-
-// isValidOperator returns whether the operator and left/right nodes are valid for comparison, if not
-// false will be returned with correct error message
-func isValidOperator(operator tick.TokenType, leftNodeType, rightNodeType ValueType) error {
-	if !tick.IsExprOperator(operator) {
-		return fmt.Errorf("return: unknown operator %v", operator)
-	}
-
-	var nodeType ValueType
+		// Right
+		typeSet, exists = result[opKey.rightType]
 
-	// Only for TRegex we determine the validity of the operator by the right node
-	if rightNodeType == TRegex {
-		nodeType = rightNodeType
-	} else {
-		nodeType = leftNodeType
-	}
+		if !exists {
+			result[opKey.rightType] = make(map[tick.TokenType]bool, 0)
+			typeSet = result[opKey.rightType]
+		}
 
-	isValid := typeToBinaryOperators[nodeType][operator]
-	if !isValid {
-		return fmt.Errorf("invalid %s %s operator %v", nodeType.String(), operatorType(operator), operator)
+		typeSet[opKey.operator] = true
 	}
 
-	return nil
-}
+	return result
+})()
 
-func operatorType(operator tick.TokenType) string {
-	if tick.IsMathOperator(operator) {
+func operatorKind(operator tick.TokenType) string {
+	switch {
+	case tick.IsMathOperator(operator):
 		return "math"
-	}
-
-	if tick.IsCompOperator(operator) {
+	case tick.IsCompOperator(operator):
 		return "comparison"
+	case tick.IsLogicalOperator(operator):
+		return "logical"
 	}
 
 	// Actually, we shouldn't get here.. because this function is called only

tick/stateful/eval_function_node_test.go

@@ -283,7 +283,7 @@ func TestEvalFunctionNode_ComplexNodes(t *testing.T) {
 func TestStatefulExpression_Integration_EvalBool_SanityCallingFunction(t *testing.T) {
 	scope := tick.NewScope()
 
-	se := mustCompileExpression(t, &tick.BinaryNode{
+	se := mustCompileExpression(&tick.BinaryNode{
 		Operator: tick.TokenEqual,
 		Left: &tick.FunctionNode{
 			Func: "count",