Optimize simple time ranged search queries

[tontinton]

When the search request contains a time range, we aborted the optimization of converting unneeded split searches into count queries.

Removed the TODO that was in the code for this.

Split the PR into 2: #5758.

[rdettai]

Thanks for your PR. The way the logic is split here between optimize_split_order and optimize makes it very hard to proof read. Currently it's already a bit confusing because the optimize() logic for each variant depends on the sort order which is different for each variant. It would be good if we could avoid tightening the coupling between the two match statements even further by making the sort orders more complex.

[rdettai]

Can you explain why this works? Say you have 5 splits:

• 1 is contained in the request
• 4 are only overlapping the request

In that case min_required_splits would be 1 here, but there might be a biggest_end_timestamp or smallest_start_timestamp in any of the other splits.

[tontinton]

My thoughts were that there's a

.take_while(|partial_sum| *partial_sum < num_requested_docs)

so min_required_splits would not get to be 1, but now I see that I should validate that we actually reached this condition, and only if we reached it to set min_required_splits and optimize, otherwise return early.

[tontinton]

I pushed a quick fix, I need to test it, but I think it answers the problem.

[rdettai]

I didn't know of std::ops::ControlFlow 😄. Nevertheless, this reaches the limit when iterators stop being practical. A for loop is much more readable here.

[tontinton]

Thanks for your PR. The way the logic is split here between optimize_split_order and optimize makes it very hard to proof read. Currently it's already a bit confusing because the optimize() logic for each variant depends on the sort order which is different for each variant. It would be good if we could avoid tightening the coupling between the two match statements even further by making the sort orders more complex.

It is a bit confusing, I'll try to think of a better way to do this when I have some more time.

[rdettai]

I would try refactoring this into:

fn optimize()
  match self {
    CandSplitDoBetter::SplitIdHigher(_) => optimize_split_id_higher()
    CandSplitDoBetter::SplitTimestampLower(_) => optimize_split_timesamp_lower()
    ...
    CanSplitDoBetter::Uninformative => {}
  }

where optimize_split_xxx:

• sorts
• returns early if !is_simple_all_query
• applies its variant specific optimization

This would regroup the optimizations logics with the sorts they depend on to be correct. (To help the review, ideally, re-implement the current logic in 1 commit, and in a separate commit add you logic extension.)

Thanks!