Fix arrive by filtering for on-street/flex itineraries

[leonardehrenfried]

Summary

It fixes a bug in the OutsideSearchWindowFilter where on street results that were shorter than transit would be removed even though they are often the best results.

cc @daniel-heppner-ibigroup

Issue

Closes #6046

Unit tests

Added.

Documentation

Javadoc.

[codecov]

Codecov Report

Attention: Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.90%. Comparing base (b174c25) to head (9cb2f4e).
Report is 234 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...request/preference/ItineraryFilterPreferences.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6050      +/-   ##
=============================================
+ Coverage      69.84%   69.90%   +0.05%     
- Complexity     17463    17698     +235     
=============================================
  Files           1974     1996      +22     
  Lines          74606    75310     +704     
  Branches        7640     7705      +65     
=============================================
+ Hits           52109    52644     +535     
- Misses         19850    19995     +145     
- Partials        2647     2671      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[daniel-heppner-ibigroup]

This fixes the issues we were seeing with arriveBy, happy to see both flex direct and other flex+transit results in the same query. I'll take a look at the code now.

[leonardehrenfried]

I removed the enum SearchDirection and will take a look at it in another PR.

[optionsome]

This is slightly difficult to understand. Maybe use bullets for listing the criteria for filtering. Also, I'm a bit on the edge if this should belong here or on the javadoc for this method as this method doesn't really contain anything else.

[leonardehrenfried]

I converted it to Javadoc and tried to make it more understandable.

[t2gran]

@leonardehrenfried Did you find out if the direct flex is only produced in the first page - the same way as other direct street routes - found by AStar?

I think the OutsideSearchWindowFilter should be even simpler than the implementation you have done. It should skip all Itineraries not produced by Raptor(witch uses the search-window). The AStar does not uses the search-window (as fare as I know), so the filter should not be applied to AStar results. In some cases Raptor computes the search-window, so it is impossible too know before it before Raptor returns.

To fix this we need to know if the search-window was used by the search. The simplest way to do it(I think) is to add flag to the itinerary, f.eks search-window-aware(should only be used by filtering, not exposed outside of OTP), set all raptor results to true and all direct searches to false. If we start using the search-window for FLEX, then we need to harmonize the search-window - I am not sure it is just a check on the latest-depature-time for arriveBy=true for the first page. Consult the paging README to go through all the corner cases - I am unsure if there are more cases to account for.

There is another problem here as well. I took a look at the SortOrderComparator - it uses the Itinerary#isOnStreetAllTheWay() for move all street results first - less likely to get cropped by the PagingFilter. So FLEX are not put first - not isOnStreetAllTheWay(), they are sorted together with regular transit - but they are misse when fetching the next page.

[leonardehrenfried]

I did a deep dive on this topic, refreshed my memory on how the paging works and added a few hints in the Javadoc about how the various parts of the paging logic work with each other.

The good news is that the logic change in this PR works well for any direct itinerary that is not flex as they can always be timeshifted to the request time. Also, only the initial page creates a direct search so the other pages are not affected.

For flex we do have a change of behaviour: the direct flex router also looks at the next day (previous for arriveBy) and potentially shows results that start a long time after the search time.

When you mix flex with transit then the time window (although not used by the flex router) acts as a sort of sanity check to give you results not too far into the future. If we go ahead with this approach then more results like these would show up.

Lets talk about it in the dev meeting.

[t2gran]

filterDirectFlexByEarliestDepartureTime ?

Is this still correct?

[t2gran]

I would prefer something like:

if (it.isDirectFlex() {
  return switch(sortOrder) {
  
  }

}