-
Notifications
You must be signed in to change notification settings - Fork 512
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.
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
Ensure binary operation is not mapped if operands are not split #2572
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (modulo a couple of nits)
@@ -184,18 +184,18 @@ func (i *instantSplitter) mapBinaryExpr(expr *parser.BinaryExpr, stats *MapperSt | |||
if err != nil { | |||
return nil, false, err | |||
} | |||
finished = lhsFinished || rhsFinished |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: what are the implications if only 1 of the 2 legs have finished? What does it mean on the other one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think no implications, one of the legs will be split and the other not. I assume the sharding middleware will then do the sharding on each leg separately.
There are also tests to binary operations with these cases:
mimir/pkg/frontend/querymiddleware/astmapper/instant_splitting_test.go
Lines 118 to 133 in 049ad98
// Binary operations | |
{ | |
in: `rate({app="foo"}[3m]) / rate({app="baz"}[6m])`, | |
out: `(sum without() (` + concatOffsets(splitInterval, 3, `increase({app="foo"}[x]y)`) + `) / 180) / (sum without() (` + concatOffsets(splitInterval, 6, `increase({app="baz"}[x]y)`) + `) / 360)`, | |
expectedSplitQueries: 9, | |
}, | |
{ | |
in: `rate({app="foo"}[3m]) / 10`, | |
out: `(sum without() (` + concatOffsets(splitInterval, 3, `increase({app="foo"}[x]y)`) + `) / 180) / (10)`, | |
expectedSplitQueries: 3, | |
}, | |
{ | |
in: `10 / rate({app="foo"}[3m])`, | |
out: `(10) / (sum without() (` + concatOffsets(splitInterval, 3, `increase({app="foo"}[x]y)`) + `) / 180)`, | |
expectedSplitQueries: 3, | |
}, |
What this PR does
Ensure binary operation is not mapped if operands are not split. This is done by checking the
stats.GetShardedQueries()
metric.Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]