-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Translate start/end to filter on @timestamp #138027
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) |
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.
Suggest an improvement in creating the filter otherwise LGTM
| return promqlPlan; | ||
| } | ||
|
|
||
| private static LogicalPlan withTimestampFilter(PromqlCommand promqlCommand, LogicalPlan plan) { |
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.
Since only one of start and end might be missing, handle each individually then assemble the filter using Predicates.combineAnd at the end:
List<Expression> timestampFilter = new ArrayList<>();
if (promqlCommand.start() ..) {
timestampFilter.add(new GreaterThan,...)
}
if (promqlCommand.end() ..) {
timestampFilter.add(new LessThan...)
}
if (timestampFilter.isEmpty() == false) {
plan = new Filter(..., plan, Predicates.combineAnd(timestampFilter)
}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.
Since only one of start and end might be missing
I don't think that can happen. We're validating that either both or none are set:
Lines 1337 to 1344 in 53436ac
| if (start == null || end == null) { | |
| throw new ParsingException( | |
| source, | |
| "Parameters [{}] and [{}] must either both be specified or both be omitted for a range query", | |
| START, | |
| END | |
| ); | |
| } |
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.