fix(query sharding): Generalize avg -> sum/count sharding using existing binop mapper #12599
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeschkies
approved these changes
Apr 12, 2024
| Op: syntax.OpTypeDiv, | ||
| }, bytesPerShard, nil | ||
|
|
||
| return m.mapBinOpExpr(binOp, r, topLevel) |
There was a problem hiding this comment.
I would have sworn that I did this. Nice find.
rhnasc
pushed a commit
to inloco/loki
that referenced
this pull request
Apr 12, 2024
This was referenced Apr 17, 2024
This was referenced May 6, 2024
This was referenced May 27, 2024
This was referenced Jun 10, 2024
This was referenced Jul 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We make
avgqueries shardable by converting them intosum/countqueries. The conversion didn't handle all possible mappings and sometimes results in the errorSelectSamples unimplemented: the query-frontend cannot evaluate an expression that selects samples. this is likely a bug in the query engine. please contact your system operator. This happens even when inputting the nativesum/countquery would succeed.For example:
avg by(foo_extracted) (quantile_over_time(0.95, {foo="baz"} | logfmt | unwrap foo_extracted | __error__=`` [$__auto]))failssum by(foo_extracted) (quantile_over_time(0.95, {foo="baz"} | logfmt | unwrap foo_extracted | __error__=`` [$__auto])) / count by(foo_extracted) (quantile_over_time(0.95, {foo="baz"} | logfmt | unwrap foo_extracted | __error__=`` [$__auto]))succeeds.This happens because our handling of top-level binary-operations is pretty sophisticated but the conversion from
avg -> sum/countwas only a piece of that. This PR removes the one-off mapping of binops in theavg -> sum/countcase and replaces it with a bin op that is handed by our normalmapBinOplogic.