Tweak traceql parsing to correctly parse binary operations without sp…

…ace (#1941) * Tweak traceql parsing to correctly parse some binary operations without space * changelog
grafana · Dec 7, 2022 · e9b4391 · e9b4391
1 parent a3e84a6
commit e9b4391
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -100,6 +100,7 @@ Internal types are updated to use `scope` instead of `instrumentation_library`.
 * [BUGFIX] tempo-mixin: tweak dashboards to support metrics without `cluster` label present [#1913](https://github.com/grafana/tempo/pull/1913) (@kvrhdn)
 * [ENHANCEMENT] New tenant dashboard [#1901](https://github.com/grafana/tempo/pull/1901) (@mapno)
 * [BUGFIX] Fix docker-compose examples not running on Apple M1 hardware [#1920](https://github.com/grafana/tempo/pull/1920) (@stoewer)
+* [BUGFIX] Fix traceql parsing of most binary operations to not require spacing [#1939](https://github.com/grafana/tempo/pull/1941) (@mdisibio)
 
 ## v1.5.0 / 2022-08-17
 

diff --git a/pkg/traceql/lexer.go b/pkg/traceql/lexer.go
@@ -224,12 +224,16 @@ func isDurationRune(r rune) bool {
 }
 
 func isAttributeRune(r rune) bool {
-	return !unicode.IsSpace(r) &&
-		r != scanner.EOF &&
-		r != '(' &&
-		r != ')' &&
-		r != '}' &&
-		r != '{'
+	if unicode.IsSpace(r) {
+		return false
+	}
+
+	switch r {
+	case scanner.EOF, '{', '}', '(', ')', '=', '~', '!', '<', '>', '&', '|', '^':
+		return false
+	default:
+		return true
+	}
 }
 
 func startsAttribute(tok int) bool {

diff --git a/pkg/traceql/lexer_test.go b/pkg/traceql/lexer_test.go
@@ -134,3 +134,9 @@ func testLexer(t *testing.T, tcs []lexerTestCase) {
 		})
 	}
 }
+
+func BenchmarkIsAttributeRune(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		isAttributeRune('=')
+	}
+}
diff --git a/pkg/traceql/parse_test.go b/pkg/traceql/parse_test.go
@@ -1,6 +1,7 @@
 package traceql
 
 import (
+	"strings"
 	"testing"
 	"time"
 
@@ -646,36 +647,48 @@ func TestSpansetFilterStatics(t *testing.T) {
 
 func TestSpansetFilterOperators(t *testing.T) {
 	tests := []struct {
-		in       string
-		err      error
-		expected FieldExpression
+		in                   string
+		expected             FieldExpression
+		alsoTestWithoutSpace bool
 	}{
 		{in: "{ .a + .b }", expected: newBinaryOperation(OpAdd, NewAttribute("a"), NewAttribute("b"))},
 		{in: "{ .a - .b }", expected: newBinaryOperation(OpSub, NewAttribute("a"), NewAttribute("b"))},
 		{in: "{ .a / .b }", expected: newBinaryOperation(OpDiv, NewAttribute("a"), NewAttribute("b"))},
 		{in: "{ .a % .b }", expected: newBinaryOperation(OpMod, NewAttribute("a"), NewAttribute("b"))},
 		{in: "{ .a * .b }", expected: newBinaryOperation(OpMult, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a = .b }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a != .b }", expected: newBinaryOperation(OpNotEqual, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a =~ .b }", expected: newBinaryOperation(OpRegex, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a !~ .b }", expected: newBinaryOperation(OpNotRegex, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a > .b }", expected: newBinaryOperation(OpGreater, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a >= .b }", expected: newBinaryOperation(OpGreaterEqual, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a < .b }", expected: newBinaryOperation(OpLess, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a <= .b }", expected: newBinaryOperation(OpLessEqual, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a ^ .b }", expected: newBinaryOperation(OpPower, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a && .b }", expected: newBinaryOperation(OpAnd, NewAttribute("a"), NewAttribute("b"))},
-		{in: "{ .a || .b }", expected: newBinaryOperation(OpOr, NewAttribute("a"), NewAttribute("b"))},
+		{in: "{ .a = .b }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a != .b }", expected: newBinaryOperation(OpNotEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a =~ .b }", expected: newBinaryOperation(OpRegex, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a !~ .b }", expected: newBinaryOperation(OpNotRegex, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a > .b }", expected: newBinaryOperation(OpGreater, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a >= .b }", expected: newBinaryOperation(OpGreaterEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a < .b }", expected: newBinaryOperation(OpLess, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a <= .b }", expected: newBinaryOperation(OpLessEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a ^ .b }", expected: newBinaryOperation(OpPower, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a && .b }", expected: newBinaryOperation(OpAnd, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
+		{in: "{ .a || .b }", expected: newBinaryOperation(OpOr, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true},
 		{in: "{ !.b }", expected: newUnaryOperation(OpNot, NewAttribute("b"))},
 		{in: "{ -.b }", expected: newUnaryOperation(OpSub, NewAttribute("b"))},
+
+		// Against statics
+		{in: "{ .a = `foo` }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticString("foo")), alsoTestWithoutSpace: true},
+		{in: "{ .a = 3 }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticInt(3)), alsoTestWithoutSpace: true},
+		{in: "{ .a = 3.0 }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticFloat(3)), alsoTestWithoutSpace: true},
+		{in: "{ .a = true }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticBool(true)), alsoTestWithoutSpace: true},
+	}
+
+	test := func(q string, expected FieldExpression) {
+		actual, err := Parse(q)
+		require.NoError(t, err, q)
+		require.Equal(t, &RootExpr{newPipeline(newSpansetFilter(expected))}, actual, q)
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.in, func(t *testing.T) {
-			actual, err := Parse(tc.in)
-
-			require.NoError(t, err)
-			require.Equal(t, &RootExpr{newPipeline(newSpansetFilter(tc.expected))}, actual)
+			test(tc.in, tc.expected)
+			if tc.alsoTestWithoutSpace {
+				test(strings.ReplaceAll(tc.in, " ", ""), tc.expected)
+			}
 		})
 	}
 }