TraceQL: Nested set intrinsics #3497
Conversation
joe-elliott
requested review from
annanay25,
mdisibio,
mapno,
kvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
| @@ -1209,6 +1224,35 @@ func createSpanIterator(makeIter makeIterFn, primaryIter parquetquery.Iterator, | |||
| case traceql.IntrinsicStructuralSibling: | |||
| selectColumnIfNotAlready(columnPathSpanParentID) | |||
| continue | |||
|
|
|||
| case traceql.IntrinsicNestedSetLeft: | |||
| nestedSetLeftExplicit = true | |||
There was a problem hiding this comment.
Can you check the behavior in this area, and see if these preds work ok with the nils added in the cases above? I'm thinking about a query that invokes both e.g. { nestedSetLeft = 1 } >> { } | select(nestedSetRight).
There was a problem hiding this comment.
this works b/c the columns are stored in the columnPredicates and columnSelectAs maps.
if the explicit intrinsics hit first then they are added to the map and then the logic in selectColumnIfNotAlready prevents them from being overwritten with an OpNone condition.
if the structural intrinsics hit first then they are overwritten when the explicit intrinsics come along.
There was a problem hiding this comment.
i did note and fix an issue in this area. if the structural intrinsic were to be added after the nested set intrinsic then it would have seen a non-empty predicate and not added the nil predicate. added to test for nil predicate
| }, | ||
| // fun way to get the root span | ||
| { | ||
| req: &tempopb.SearchRequest{Query: "{ nestedSetParent = -1 } | select(name)"}, |
There was a problem hiding this comment.
Can you add a case for when we dual purpose read these columns. Similar to the other comment a query like: {nestedSetParent = -1} >> {} | select(name)
There was a problem hiding this comment.
so there is kind of a bug here, but it exists for all spanset operators. for instance this:
{ span.foo = "bar" } >> {}
Will return the value of the attribute "foo" on every matched span even though the condition is on the LHS of the operator. I'm not sure if this is a bug or not.
There was a problem hiding this comment.
Agree it sounds like a bug. But pre-existing so ok for this PR, if I understand you correctly.
| @@ -141,6 +141,16 @@ func (s *span) AttributeFor(a traceql.Attribute) (traceql.Static, bool) { | |||
| } | |||
|
|
|||
| if a.Intrinsic != traceql.IntrinsicNone { | |||
| if a.Intrinsic == traceql.IntrinsicNestedSetLeft { | |||
There was a problem hiding this comment.
Do you know if these extra checks are enough to show up in benchmarks, or would a switch be ok here? Wondering at what point this method needs another overhaul. For instance there is duplicate logic for .foo fallback to check resource-level and then span-level, between here and engine.
There was a problem hiding this comment.
i will check benches but would guess these are not visible. don't mind swapping to a switch either for aesthetic or perf reasons if that's preferred.
are switches faster than a series of ifs?
joe-elliott
force-pushed
the
nested-set-queries
branch
from
21f3f62
to
48b159b
Compare
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
joe-elliott
force-pushed
the
nested-set-queries
branch
from
48b159b
to
104cf8d
Compare
Expose nested set intrinsics into the traceql language. This allows calling applications to request structural details about spans.