-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Parse start/end/time/step parameters #137472
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
Parse start/end/time/step parameters #137472
Conversation
The responses in PromQL look different for these (`value` vs `values`)
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/promql/PromqlParams.java
Outdated
Show resolved
Hide resolved
.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/promql/PromqlParserUtils.java
Outdated
Show resolved
Hide resolved
costin
left a comment
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.
Left a round of comments
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 should now fixed - please verify if that's the case on your end as well.
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.
Some tests in PromqlLogicalPlanOptimizerTests are still failing (before and after this change). Maybe you meant PromqlVerifierTests? Some are working now but not all.
| } | ||
|
|
||
| public void testValidInstantQuery() { | ||
| PromqlCommand promql = parse("TS test | PROMQL time `2025-10-31T00:00:00Z` (avg(foo))"); |
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.
foo is a quoted identifier that is a reference to a field which the parser should translate into an UnresolvedAttribute(foo).
If you want to parse it as a value use define it as a string - "2025..."
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.
I'll follow up on that shortly. I'd like to do that in a separate change as it needs changes to the grammar.
…to promql-parameter-parsing
BASE=2351e395658a305869ca62d07951a0ff036f8d79 HEAD=24e35edae64567854d1b1e47ca3abd1a1691a571 Branch=esql/promql
BASE=2351e395658a305869ca62d07951a0ff036f8d79 HEAD=24e35edae64567854d1b1e47ca3abd1a1691a571 Branch=esql/promql
Only parses the parameters, doesn't actually do anything with them, yet.
I'm hoping we can make start/end optional and a consequence of not specifying is that the
start()andend()functions return an error.This seems like a sensible pattern when used within Kibana, for example when creating an ES|QL-based visualization. Kibana is already supplying a filter for the time range outside of the query itself, so having to specify start/end would be redundant. The start/end parameters can still be used within and outside of Kibana but will likely mostly be useful outside of Kibana, and act similar to
TRANGE.We'll need to solve the issue of increasing the time window we load data from. But I see this as a separate issue that we should solve when tackling window functions. This includes handling the complexity of mixing start/end, time filtering using
filter(as Kibana does), and TBUCKET/WHERE filtering in the same query.Examples: