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.
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.
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.
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.