-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Translate start/end to filter on @timestamp
#138412
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
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| query = query.replace("$now", '"' + Instant.now().toString() + '"'); | ||
| var analyzed = tsAnalyzer.analyze(parser.createStatement(query)); | ||
| logger.trace(analyzed); | ||
| logger.trace("analyzed plan:\n{}", analyzed); |
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.
Remove? Or set to debug? Same below.
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 think the trace level is fine to be able to debug/inspect the plan during testing. I've removed the redundant logger.trace(plan) in all of the other tests as we're doing that in here already.
| List<LogicalPlan> collect = plan.collect(Bucket.class::isInstance); | ||
| logger.trace(plan); | ||
| var filters = plan.collect(Filter.class::isInstance); | ||
| assertThat(filters, hasSize(1)); |
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.
Shouldn't we have 2?
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.
There's one filter with multiple conditions (metric not null, timestamp >= start, timestamp <= end)
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've adjusted the assertion in 6f88cf7 to check that there are two conditions for @timestamp.
This is a variant of #138027 where the target is main instead of the now merged esql/promql branch.
The start/end parameters are optional. If set, this creates a simple filter on the timestamp.
If start/end is not set, this doesn't create a filter and if @ start() and @ end() functions are used, it throws an error during parsing.
We don't attempt to infer start/end from other filters and we don't do anything special if there's another filter on @timestamp.
For instant queries where start=end=time, this essentially creates a filter that only includes data for a particular instant. We'll need to wire the lookback that will come after the initial window function support later on. Right now, instant queries will essentially always return an empty result. Maybe we just add a verifier rule to disallow instant queries.