-
Notifications
You must be signed in to change notification settings - Fork 9.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
Making durations and number literals the same #9138
Conversation
a062c61
to
724ad46
Compare
Thank you. I expect that Beorn will want to look at this one, but you can expect some delays due to the summer period. |
According to your design doc, the following should work: |
I tried
I also left some comments on the design doc on questions I had. |
One thing I'm interested in is how this will affect the printing of PromQL expressions once they have been parsed. It would be sad if the input was:
...and we could only serialize it out again as:
Not sure if this is the plan already, but would it make sense to store the original tokens in the parsed AST somehow so that we can render them out again in a way that makes most sense to the user? |
That's a good point, but perhaps we can solve that later, once the main functionality is implemented. I think the comments above( #9138 (comment) , #9138 (comment) ) are real issues that need to be fixed. |
00f908b
to
20fdee0
Compare
thank you, will address 👍 |
3094b01
to
89c8d21
Compare
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 might come from my ignorance of parser internals, but wouldn't it be easier to just get rid of the DURATION type altogether and replace it with NUMBER everywhere? And then parse each number as a duration.
FYI, VictoriaMetrics supports using durations instead of numeric values and vice versa in MetricsQL. See, for example, how does |
We discussed this during our bug scrub. I think my point above is still valid and hasn't been addressed. @darshanime are you still up to working on this? This general topic comes up quite often, so we should finish this up one way or another. |
89c8d21
to
2e3d90a
Compare
Signed-off-by: darshanime <[email protected]>
Signed-off-by: darshanime <[email protected]>
Signed-off-by: darshanime <[email protected]>
Signed-off-by: darshanime <[email protected]>
49eeb20
to
1a1b09f
Compare
Signed-off-by: beorn7 <[email protected]>
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.
Rebased and adjusted the version (as this will land in 2.54, not 2.53).
/prombench main |
/prombench cancel |
Benchmark cancel is in progress. |
Sorry, it's pointless comparing against main when the PR has been merged. |
Benchmark tests are running for 3 days! If this is intended ignore this message otherwise you can cancel it by commenting: |
/prombench cancel |
Benchmark cancel is in progress. |
Signed-off-by: Jan Fajerski <[email protected]> Conflicts: VERSION pick 3.0.0 promql/promqltest/testdata/histograms.test pick changes from c39776c, but adjust 5m range selectors to 10m to account for prometheus#13904. Fixes: promql/promqltest/testdata/functions.test promql/promqltest/testdata/staleness.test Tests added in prometheus#9138 need to be adjusted to account for prometheus#13904.
Signed-off-by: Bryan Boreham <[email protected]>
Design doc: https://docs.google.com/document/d/1LaZfknXuuRWGtQSbULoMtclQhuLUMrdwg15wMvoBvCQ/edit#
Signed-off-by: darshanime [email protected]