Fix certain binary expression queries by optimizing through push down #6132
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
Instant queries of the following type fail and return an `unimplemented`
error:
```
sum(count_over_time({foo="bar"} | logfmt | duration > 2s [3s]))
/
sum(count_over_time({foo="bar"} [3s]))
```
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
If either the left hand side or the right hand side of a binary expression is a noop, we need to return the original expression so the whole expression is a noop as well, and thus not executed using the downstream engine. Otherwise, a binary expression that has a noop on either side, results in an `unimplemented` error when executed using the downstream engine. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
This change optimizes queries that use a vector aggregation without grouping around a range aggregation with a label extraction stage such as `json` or `logfmt`. Since the vector aggregation can be pushed down to the downstream query, the downstream query does not create a massive amount of streams, even though it contains a generic label extraction stage. Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
- logql -0.1%
+ loki 0% |
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
sandeepsukhani
pushed a commit
that referenced
this pull request
May 12, 2022
…#6132) * Add test cases that fail Instant queries of the following type fail and return an `unimplemented` error: ``` sum(count_over_time({foo="bar"} | logfmt | duration > 2s [3s])) / sum(count_over_time({foo="bar"} [3s])) ``` Signed-off-by: Christian Haudum <christian.haudum@gmail.com> * Fix certain binary expressions for instant queries If either the left hand side or the right hand side of a binary expression is a noop, we need to return the original expression so the whole expression is a noop as well, and thus not executed using the downstream engine. Otherwise, a binary expression that has a noop on either side, results in an `unimplemented` error when executed using the downstream engine. Signed-off-by: Christian Haudum <christian.haudum@gmail.com> * Optimize instant vector aggregations with log extraction stage This change optimizes queries that use a vector aggregation without grouping around a range aggregation with a label extraction stage such as `json` or `logfmt`. Since the vector aggregation can be pushed down to the downstream query, the downstream query does not create a massive amount of streams, even though it contains a generic label extraction stage. Signed-off-by: Christian Haudum <christian.haudum@gmail.com> * fixup! Add test cases that fail Signed-off-by: Christian Haudum <christian.haudum@gmail.com> * fixup! fixup! Add test cases that fail Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
This was referenced Jun 28, 2022
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.
What this PR does / why we need it:
This PR optimizes instant queries that use a vector aggregation around a range aggregation with log extraction stage (
json,logfmt), such assum(count_over_time({foo="bar"} | logfmt | duration > 30s [24h])).Since the vector aggregation can be pushed down to the downstream query, the downstream query does not create a massive amount of streams, even though it contains a generic label extraction stage.
This would also fix binary expression queries that use said aggregation on one of its leave nodes, such as:
If either the left hand side or the right hand side of a binary expression is a noop, we need to return the original expression so the whole expression is a noop as well, and thus not executed using the downstream engine.
Otherwise, a binary expression that has a noop on either side, results in an
unimplementederror when executed using the downstream engine.Which issue(s) this PR fixes:
Fixes #6130
Special notes for your reviewer:
Checklist
CHANGELOG.md.docs/sources/upgrading/_index.md