PromQL parser: support underscores in literal numbers #12821
Conversation
|
I will consult the team about this. |
|
@roidelapluie Has there been any feedback for this yet? If not, lets kick off a thread in prometheus-developers / dev-summit? We looked at this in our bug scrub. |
|
@rexagod sorry for letting this fall through the cracks. We have picked this up, everyone seems fine with the change. Are you still up for finishing the PR? |
|
@beorn7 Sure, apologies for the delay here. I'll patch this up. |
bfe5f35 to
97cc304
Compare
promql/parser/lex.go
Outdated
| dotPattern := "." | ||
| exponentPattern := "eE" | ||
| underscorePattern := "_" | ||
| // Anti-patterns are rune sets that cannot follow their respective rune. | ||
| dotAntiPattern := "_." | ||
| exponentAntiPattern := "._eE" // and EOL. | ||
| underscoreAntiPattern := "._eE" // and EOL. |
There was a problem hiding this comment.
These are not modified like digitPattern is. Let's move them to the package scope as const values.
There was a problem hiding this comment.
+1 on the const change, but I'm not sure about changing the scope for these (since they are only used in this method)? LMK if you think otherwise and I'll make the change.
There was a problem hiding this comment.
The code looks good. Thank you very much.
However, this needs an update of the frontend, too. Otherwise, the syntax highlighting will not work properly for numbers with underscores, and the UI will draw squiggly red lines:
I'm not a frontend expert myself (@juliusv and @Nexucis are the usual persons to ask for help), but I guess the changes will be in the web/ui/module/lezer-promql/src/ directory.
promql/parser/lex.go
Outdated
| @@ -313,6 +313,11 @@ func (l *Lexer) accept(valid string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // is peeks and returns true if the next rune is in the valid set. | |||
There was a problem hiding this comment.
From the doc comment, I found it a bit hard to understand what this helper method is doing. I had to read the code. Given that the code is short and the method is unexported, I would be fine without a doc comment. But if we keep it, it should probably be clearer, e.g. "is is peeks and returns true if if the next rune is contained in the provided string".
|
@rexagod do you think you can tackle the UI changes, too? We could also try to find a frontend expert to do it, but that's notoriously hard. |
|
Yeah, you probably have to just edit prometheus/web/ui/module/lezer-promql/src/promql.grammar Lines 263 to 266 in c5040c5 make build process should take care of the rest (parser regeneration, etc.).
|
Support underscores in numbers, namely, decimals, hexadecimals, and exponentials. Fixes prometheus#12769 Signed-off-by: Pranshu Srivastava <rexagod@gmail.com> Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
|
OK, time is up. Merging… 💥 |
Support underscores in numbers, namely, decimals, hexadecimals, and exponentials.
Fixes #12769.