Skip to content

TraceQL: make comparison to nil symmetric #4869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Mar 24, 2025
Merged

TraceQL: make comparison to nil symmetric #4869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7e28eb2
Add test case for symmetric .foo != nil
stoewer Mar 18, 2025
3ce0c81
Fix make .foo != nil symmetric
stoewer Mar 18, 2025
b7e3af8
Adjust test for isMatchingOperand()
stoewer Mar 18, 2025
5915ea4
Fix false matches for {} | avg(.dne) != 0
stoewer Mar 20, 2025
7986ec5
More test cases for symmetric .foo != nil
stoewer Mar 20, 2025
212600e
More cases for symmetric Static.Equals()
stoewer Mar 20, 2025
21ae368
Test cases for queries with aggregates over non existing attrs
stoewer Mar 20, 2025
d2517d6
Fix behavior of aggregates over non existing attrs
stoewer Mar 21, 2025
3abf776
CHANGELOG.md
stoewer Mar 21, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ configurable via the throughput_bytes_slo field, and it will populate op="traces
* [ENHANCEMENT] distributor: add IPv6 support [#4840](https://github.com/grafana/tempo/pull/4840) (@gjacquet)
* [BUGFIX] Choose a default step for a gRPC streaming query range request if none is provided. [#4546](https://github.com/grafana/tempo/pull/4576) (@joe-elliott)
Correctly copy exemplars for metrics like `| rate()` when gRPC streaming.
* [BUGFIX] Make comparison to nil symmetric [#4869](https://github.com/grafana/tempo/pull/4869) (@stoewer)
* [BUGFIX] Fix behavior for queries like {.foo && true} and {.foo || false} [#4855](https://github.com/grafana/tempo/pull/4855) (@stoewer)
* [BUGFIX] Fix performance bottleneck and file cleanup in block builder [#4550](https://github.com/grafana/tempo/pull/4550) (@mdisibio)
* [BUGFIX] TraceQL incorrect results for additional spanset filters after a select operation [#4600](https://github.com/grafana/tempo/pull/4600) (@mdisibio)
Expand Down
4 changes: 4 additions & 0 deletions pkg/traceql/ast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,10 @@ func (s Static) StrictEquals(o *Static) bool {
}
}

func (s Static) IsNil() bool {
return s.Type == TypeNil
}

func (s Static) compare(o *Static) int {
if s.Type != o.Type {
if s.isNumeric() && o.isNumeric() {
Expand Down
34 changes: 33 additions & 1 deletion pkg/traceql/ast_execute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,9 @@ func (f ScalarFilter) evaluate(input []*Spanset) (output []*Spanset, err error)
return output, nil
}

func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) {
func (a Aggregate) evaluate(input []*Spanset) ([]*Spanset, error) {
var output []*Spanset

for _, ss := range input {
switch a.op {
case aggregateCount:
Expand All @@ -253,6 +255,9 @@ func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) {
if err != nil {
return nil, err
}
if val.IsNil() {
continue
}

if sum == nil {
sum = &val
Expand All @@ -261,6 +266,9 @@ func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) {
}
count++
}
if sum == nil {
continue
}

cpy := ss.clone()
cpy.Scalar = sum.divideBy(float64(count))
Expand All @@ -274,10 +282,18 @@ func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) {
if err != nil {
return nil, err
}
if val.IsNil() {
continue
}

if maxS == nil || val.compare(maxS) > 0 {
maxS = &val
}
}
if maxS == nil {
continue
}

cpy := ss.clone()
cpy.Scalar = *maxS
cpy.AddAttribute(a.String(), cpy.Scalar)
Expand All @@ -290,10 +306,18 @@ func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) {
if err != nil {
return nil, err
}
if val.IsNil() {
continue
}

if minS == nil || val.compare(minS) == -1 {
minS = &val
}
}
if minS == nil {
continue
}

cpy := ss.clone()
cpy.Scalar = *minS
cpy.AddAttribute(a.String(), cpy.Scalar)
Expand All @@ -306,12 +330,20 @@ func (a Aggregate) evaluate(input []*Spanset) (output []*Spanset, err error) {
if err != nil {
return nil, err
}
if val.IsNil() {
continue
}

if sum == nil {
sum = &val
} else {
sum.sumInto(&val)
}
}
if sum == nil {
continue
}

cpy := ss.clone()
cpy.Scalar = *sum
cpy.AddAttribute(a.String(), cpy.Scalar)
Expand Down
15 changes: 14 additions & 1 deletion pkg/traceql/ast_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,12 +473,15 @@ func TestStatic_Equals(t *testing.T) {
{NewStaticInt(1), NewStaticInt(1)},
{NewStaticFloat(1.5), NewStaticFloat(1.5)},
{NewStaticInt(2), NewStaticFloat(2.0)},
{NewStaticFloat(2.0), NewStaticInt(2)},
{NewStaticString("foo"), NewStaticString("foo")},
{NewStaticBool(true), NewStaticBool(true)},
{NewStaticBool(false), NewStaticBool(false)},
{NewStaticDuration(1 * time.Second), NewStaticDuration(1000 * time.Millisecond)},
{NewStaticDuration(0), NewStaticInt(0)},
{NewStaticInt(0), NewStaticDuration(0)},
{NewStaticStatus(StatusOk), NewStaticStatus(StatusOk)},
{NewStaticKind(KindClient), NewStaticKind(KindClient)},
{NewStaticDuration(0), NewStaticInt(0)},
{NewStaticIntArray([]int{}), NewStaticIntArray(nil)},
{NewStaticIntArray([]int{11, 111}), NewStaticIntArray([]int{11, 111})},
{NewStaticFloatArray([]float64{}), NewStaticFloatArray(nil)},
Expand All @@ -491,18 +494,28 @@ func TestStatic_Equals(t *testing.T) {
{NewStaticStatus(StatusError), NewStaticInt(0)},
{NewStaticStatus(StatusOk), NewStaticInt(1)},
{NewStaticStatus(StatusUnset), NewStaticInt(2)},
{NewStaticInt(2), NewStaticStatus(StatusUnset)},
}
areNotEqual := []struct {
lhs, rhs Static
}{
{NewStaticInt(1), NewStaticInt(2)},
{NewStaticInt(1), StaticNil},
{StaticNil, NewStaticInt(1)},
{NewStaticBool(true), NewStaticBool(false)},
{NewStaticBool(true), NewStaticInt(1)},
{NewStaticInt(1), NewStaticBool(true)},
{NewStaticString("foo"), NewStaticString("bar")},
{NewStaticString(""), StaticNil},
{StaticNil, NewStaticString("")},
{NewStaticKind(KindClient), NewStaticKind(KindConsumer)},
{NewStaticStatus(StatusError), NewStaticStatus(StatusOk)},
{NewStaticStatus(StatusOk), NewStaticStatus(StatusUnset)},
{NewStaticStatus(StatusOk), NewStaticKind(KindInternal)},
{NewStaticStatus(StatusError), NewStaticFloat(0)},
{NewStaticFloat(0), NewStaticStatus(StatusError)},
{NewStaticStatus(StatusError), StaticNil},
{StaticNil, NewStaticStatus(StatusError)},
{NewStaticIntArray([]int{}), NewStaticIntArray([]int{0})},
{NewStaticIntArray([]int{111, 11}), NewStaticIntArray([]int{11, 111})},
{NewStaticFloatArray([]float64{}), NewStaticFloatArray([]float64{0.0})},
Expand Down
2 changes: 1 addition & 1 deletion pkg/traceql/enum_statics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (t StaticType) isMatchingOperand(otherT StaticType) bool {
return true
}

if otherT == TypeNil {
if t == TypeNil || otherT == TypeNil {
return true
}

Expand Down
15 changes: 8 additions & 7 deletions pkg/traceql/enum_statics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ func TestStaticType_isMatchingOperand(t *testing.T) {
{t: TypeNil, otherT: TypeFloatArray, want: true},
{t: TypeNil, otherT: TypeStringArray, want: true},
{t: TypeNil, otherT: TypeBooleanArray, want: true},
{t: TypeNil, otherT: TypeInt, want: false},
{t: TypeNil, otherT: TypeFloat, want: false},
{t: TypeNil, otherT: TypeString, want: false},
{t: TypeNil, otherT: TypeBoolean, want: false},
{t: TypeNil, otherT: TypeDuration, want: false},
{t: TypeNil, otherT: TypeStatus, want: false},
{t: TypeNil, otherT: TypeKind, want: false},
{t: TypeNil, otherT: TypeInt, want: true},
{t: TypeNil, otherT: TypeFloat, want: true},
{t: TypeNil, otherT: TypeString, want: true},
{t: TypeNil, otherT: TypeBoolean, want: true},
{t: TypeNil, otherT: TypeDuration, want: true},
{t: TypeNil, otherT: TypeStatus, want: true},
{t: TypeNil, otherT: TypeKind, want: true},

// array types
{t: TypeIntArray, otherT: TypeIntArray, want: true},
Expand All @@ -96,6 +96,7 @@ func TestStaticType_isMatchingOperand(t *testing.T) {
name := fmt.Sprintf("%s with %s", tt.t, tt.otherT)
t.Run(name, func(t *testing.T) {
assert.Equalf(t, tt.want, tt.t.isMatchingOperand(tt.otherT), "isMatchingOperand: %s", name)
assert.Equalf(t, tt.want, tt.otherT.isMatchingOperand(tt.t), "isMatchingOperand: %s", name)
})
}
}
Expand Down
5 changes: 5 additions & 0 deletions tempodb/encoding/vparquet4/block_traceql_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ func TestBackendBlockSearchTraceQL(t *testing.T) {
{"instrumentation:name", traceql.MustExtractFetchSpansRequestWithMetadata(`{instrumentation:name = "scope-1"}`)},
{"instrumentation:version", traceql.MustExtractFetchSpansRequestWithMetadata(`{instrumentation:version = "version-1"}`)},
{"instrumentation.attr-str", traceql.MustExtractFetchSpansRequestWithMetadata(`{instrumentation.scope-attr-str = "scope-attr-1"}`)},
// Operations containing nil
{".foo != nil", traceql.MustExtractFetchSpansRequestWithMetadata(`{.foo != nil}`)},
{"nil != .foo", traceql.MustExtractFetchSpansRequestWithMetadata(`{nil != .foo}`)},
{"span.http.status_code != nil", traceql.MustExtractFetchSpansRequestWithMetadata(`{span.http.status_code != nil}`)},
{"nil != span.http.status_code", traceql.MustExtractFetchSpansRequestWithMetadata(`{nil != span.http.status_code}`)},
Comment on lines +168 to +172
Copy link
Contributor Author

@stoewer stoewer Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll consolidate those test cases with similar test cases from #4864 once the other PR is merged
// Basic data types and operations
{".float = 456.78", traceql.MustExtractFetchSpansRequestWithMetadata(`{.float = 456.78}`)}, // Float ==
{".float != 456.79", traceql.MustExtractFetchSpansRequestWithMetadata(`{.float != 456.79}`)}, // Float !=
Expand Down
15 changes: 15 additions & 0 deletions tempodb/tempodb_search_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,11 @@ func advancedTraceQLRunner(t *testing.T, wantTr *tempopb.Trace, wantMeta *tempop
// avgs
{Query: "{ } | avg(.dne) != 0"},
{Query: "{ } | avg(duration) < 0"},
{Query: "{ } | min(.dne) != 0"},
{Query: "{ } | min(duration) < 0"},
{Query: "{ } | max(.dne) != 0"},
{Query: "{ } | max(duration) < 0"},
{Query: "{ } | sum(.dne) != 0"},
{Query: "{ } | sum(duration) < 0"},
// groupin' (.foo is a known attribute that is the same on both spans)
{Query: "{} | by(span.foo) | count() = 1"},
Expand Down Expand Up @@ -1241,12 +1244,24 @@ func traceQLExistence(t *testing.T, _ *tempopb.Trace, _ *tempopb.TraceSearchMeta
key: "duration",
},
},
{
req: &tempopb.SearchRequest{Query: "{ nil != duration }", Limit: 10},
expected: expected{
key: "duration",
},
},
{
req: &tempopb.SearchRequest{Query: "{ resource.service.name != nil }", Limit: 10},
expected: expected{
key: "resource.service.name",
},
},
{
req: &tempopb.SearchRequest{Query: "{ nil != resource.service.name }", Limit: 10},
expected: expected{
key: "resource.service.name",
},
},
}
// TODO re-enable commented searches after fixing structural operator bugs in vParquet3
// https://github.com/grafana/tempo/issues/2674
Expand Down