Remove timerange root search #5760
Conversation
| pub(crate) fn extract_start_end_timestamp_from_ast( | ||
| query_ast: QueryAst, | ||
| timestamp_field: &str, | ||
| start_timestamp: &mut Option<i64>, | ||
| end_timestamp: &mut Option<i64>, | ||
| ) -> QueryAst { |
There was a problem hiding this comment.
This signature is confusing as it is both transforming an owned input and mutating references.
There was a problem hiding this comment.
Hmm, do you maybe prefer:
pub(crate) fn extract_start_end_timestamp_from_ast(
query_ast: QueryAst,
timestamp_field: &str,
start_timestamp: Option<i64>,
end_timestamp: Option<i64>,
) -> (QueryAst, Option<i64>, Option<i64>) {?
There was a problem hiding this comment.
I think the gain from making both the boundary refinement and the extraction at once is marginal, and it would make this PR a lot easier to review (and the code easier to read) to keep refine_start_end_timestamp_from_ast() as is and add a separate method for the extraction.
There was a problem hiding this comment.
The problem is with transform accepting a value and not borrow of QueryAst, meaning I would need to clone to keep the old signature of refine_start_end_timestamp_from_ast which accepts a &QueryAst, are you ok with this clone?
tontinton
force-pushed
the
remove-timerange-root-search
branch
2 times, most recently
from
a8afa5d to
28e5e76
Compare
To allow for the optimization stage to run if the resulting ast after extracting the time range is a simple query.
Does the same as RemoveTimestampRange, but even does a little more.
tontinton
force-pushed
the
remove-timerange-root-search
branch
from
28e5e76 to
d10c70c
Compare
Extract and remove time range from query ast.
To allow for the optimization stage to run if the resulting ast after extracting the time range is a simple query.