-
Notifications
You must be signed in to change notification settings - Fork 334
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
feat(promql): parameterize lookback #3630
feat(promql): parameterize lookback #3630
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.
Seems we need some integration tests.
Does this patch still need some changes on the parser side?
I think it's more or less ready. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3630 +/- ##
==========================================
- Coverage 85.38% 85.04% -0.34%
==========================================
Files 963 963
Lines 160719 160730 +11
==========================================
- Hits 137231 136700 -531
- Misses 23488 24030 +542 |
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.
Generally looks good. Suggest we convey to the HTTP endpoints so we don't leave more TODOs.
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.
Further update gRPC codepath and lift step
param in HTTP param can be follow-ups.
@tisonkun Thank you for the general feedback! I reflected on that and I'd like to spend a bit more time and deliver something useful rather then a half-baked solution. As for grpc change I'll try making adjustment in greptime-proto repo |
@etolbakov Cool! You can ping me as a reviewer when you submit a patch there. |
@etolbakov you may update the lock file locally. This should be automatically updated if you're using VSCode or RustRover. Or you can run |
Will this patch close #3453? |
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.
This is very neat, thanks @etolbakov 🚀
Let's merge this after CI passes.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#3453
What's changed and what's your intention?
Parameterize
lookback
and allow it to propagate fromTQL
Checklist