[ES|QL] PromQL pretty-printing support#247399
Conversation
|
/ci |
|
Pinging @elastic/kibana-esql (Team:ESQL) |
sddonne
left a comment
There was a problem hiding this comment.
Nice!
Code looks correct, left some suggestions and some doubts, feel free to decide if they makes sense or not :)
|
|
||
| return PromQLBuilder.expression.literal.string(ctx.getText(), '', this.getParserFields(ctx)); | ||
| const text = ctx.getText(); | ||
| return PromQLBuilder.expression.literal.string('', text, this.getParserFields(ctx)); |
There was a problem hiding this comment.
What's the reason for the value unquoted to be empty string?
There was a problem hiding this comment.
Because PromQLBuilder can construct the AST node from two patterns: (1) parsed from source text; (2) created programmatically without the source text. In the first case the second argument text is passed and the "unquoted" version is figured out by the builder. In the second case, the actual string (the unquoted version) is passed and the pretty-printer later figures out how to quote it, if at all.
| }); | ||
| }); | ||
|
|
||
| describe('synthetic AST (Builder-constructed)', () => { |
There was a problem hiding this comment.
Aren't this set of tests more related to the builder inner workings than the pretty printer?
I understand the usage of the pretty printer here is just to check the builder created the correct query, anyway these ones seems different from the rest of the tests in the file so maybe could be good to move them into another one, with these large files it could become difficult to find the correct place to add a new test.
Could be /builder/builder.test.ts to match the ES|QL scaffolding or /pretty_print/__tests__/print_from_builder.test.ts if you prefer to keep them here.
How do you see it?
There was a problem hiding this comment.
My idea was to test also the pretty-printing of queries that are not parsed, but constructed programmatically using the PromqlBuilder. I've put them now into a separate test suite to separate the two AST construction patterns.
| case 'unknown': | ||
| return '<unknown>'; | ||
| default: | ||
| return ''; |
There was a problem hiding this comment.
Just for my understanding, why is it better to ignore unsupported expression types and return '' rather than return the raw text?
There was a problem hiding this comment.
It is just one way of doing it. The default clause should not ever execute, so what we put there should not matter that much. Could also be the .text field, I don't have a strong opinion.
With printing the raw text there are few issues to be solved:
The raw source text is available only if we parsed the original query. If we constructed it using a builder programmatically, it would not have raw text version. Also, the way the pretty-printer is set up now, it receives only the AST node for printing, it does not receive the source text where to cut the nodes from, we would need to change the API of it to accept source text (and make it optional as programmatically constructed queries don't have the source text).
Some other issues with raw text: the .text field on AST nodes is stripped of hidden channel tokens (whitespace), so we cannot use that. But if we were to use the actual source text an cut out text from it we would have another whitespace issue: this "basic" pretty printer is setup to print everything on one line with a single space as separator, but the original source whitespace could have more than one space and more complex whitespace characters, line newline \n.
There was a problem hiding this comment.
Clear, thank you for the explanation 🙏
|
|
||
| protected formatOperator(op: string): string { | ||
| // Set operators (and, or, unless) might need case formatting | ||
| if (op === 'and' || op === 'or' || op === 'unless') { |
There was a problem hiding this comment.
We have these defined here , maybe we can define them as
export const promQLOperators = ['and', 'or', 'unless'] as const;
export type PromQLSetOperator = (typeof promQLOperators)[number];To re-use them here.
There was a problem hiding this comment.
We could put them in an array, once we need them in an array.
One thing I don't understand, is why would we infer the type from a runtime variable? If (as you mentioned) the type PromQLSetOperator is already defined, we can just use the type:
export const promQLOperators: PromQLSetOperator[] = ['and', 'or', 'unless'];There was a problem hiding this comment.
It's a way of only listing them once,
export const promQLOperators = ['and', 'or', 'unless'] as const;
export type PromQLSetOperator = (typeof promQLOperators)[number];vs
export type PromQLSetOperator = 'and' | 'or' | 'unless';
export const promQLOperators: PromQLSetOperator[] = ['and', 'or', 'unless'];then you could do
if (promQLOperators.inlcudes(op) {... instead of
if (op === 'and' || op === 'or' || op === 'unless') {
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
|
## Summary Partially addresses elastic#243932 Adds ability to transforms a PromQL AST back to text: pretty-print. Supports basic pretty printing where the whole PromQL query is printed on a single line with minimal whitespace. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
## Summary Partially addresses elastic#243932 Adds ability to transforms a PromQL AST back to text: pretty-print. Supports basic pretty printing where the whole PromQL query is printed on a single line with minimal whitespace. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
## Summary Partially addresses elastic#243932 Adds ability to transforms a PromQL AST back to text: pretty-print. Supports basic pretty printing where the whole PromQL query is printed on a single line with minimal whitespace. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Summary
Partially addresses #243932
Adds ability to transforms a PromQL AST back to text: pretty-print. Supports basic pretty printing where the whole PromQL query is printed on a single line with minimal whitespace.
Checklist