-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fix topk and bottomk operations with int <= 0 #6937
Conversation
./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% |
./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% |
./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% |
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.
Very nice, thanks for fixing this.
./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% |
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, nice explanation & fix
* Fix topk and bottomk operations with int <= 0 * Fix VectorAggregationExpr String method * Update rangemapper_test expression * Add tests * Update CHANGELOG.md
What this PR does / why we need it:
How the problem was identified
Range mapper query fails for topk/bottomk expressions with parameter 0.
When parsing an expression the current flow is:
ParseSampleExpr
from query string: https://github.com/grafana/loki/blob/main/pkg/logql/rangemapper.go#L80this operation succeeds with no error
2)
ParseSampleExpr
fromexpr.String()
: https://github.com/grafana/loki/blob/main/pkg/logql/rangemapper.go#L451expr.String()
does not consider the topk parameter 0 because of this condition: https://github.com/grafana/loki/blob/main/pkg/logql/syntax/ast.go#L927-L931incorrectly converting the expression to string
topk(sum by(geoip_country_code)(count_over_time({app="foo"} | json | ( geoip_country_code!="" , __error__="" )[15m])))
which then panic at https://github.com/grafana/loki/blob/main/pkg/logql/syntax/ast.go#L869-L871 because param is nilWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The changes in this PR:
expr.String()
to consider the first parameter of topk/bottomk even if 0Checklist
CHANGELOG.md
.docs/sources/upgrading/_index.md