-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Optimize filter for timeseries, search, and select queries #2931
Conversation
) | ||
{ | ||
if (!(query instanceof SearchQuery)) { | ||
return runner.run(query, responseContext); |
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.
we should probably throw an exception here as this should not happen
@drcrallen he doesn't need CLA. @acslk is with Imply. |
@drcrallen I haven't ran any benchmarks yet. |
@drcrallen there's really nothing major being changed in this PR. Do you feel benchmarks are necessary? |
Query<Result<SearchResultValue>> query, Map<String, Object> responseContext | ||
) | ||
{ | ||
if (!(query instanceof SearchQuery)) { |
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.
It would be super weird to get anything other than a SearchQuery here, so I think it'd be OK to just do the cast without checking instanceof. If the cast fails due to some insanity then we'll still get a pretty reasonable ClassCastException.
.build(); | ||
|
||
checkSearchQuery(query, expectedHits); | ||
checkSearchQuery(query, toolChest.mergeResults(toolChest.preMergeQueryDecoration(runner)), expectedHits); |
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.
If you add this style of checking to the base checkSearchQuery
(so it always does both checks – one on a bare runner and one on the decorated runner) then that'll improve the coverage of all search tests, which would be cool.
might as well do a postMergeQueryDecoration too.
looks good other than minor comments. I don't think benchmarking is necessary, this is just applying the existing |
👍 |
With addressed #2931 (comment), 👍 |
👍 |
Fixes #2766
Added runner that optimizes the dimension filter for TimeSeries, Search, and Select queries. Note these query types are the only queries other than TopN and GroupBy that has dimension filter. The runner is added in preMergeQueryDecoration of QueryToolChest, which should only be called on the broker once for each query.