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 97.91667% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.87%. Comparing base (9c10a0e) to head (744017f).
Report is 10 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.82%   69.87%   +0.05%     
- Complexity     17419    17445      +26     
=============================================
  Files           1974     1975       +1     
  Lines          74545    74576      +31     
  Branches        7633     7634       +1     
=============================================
+ Hits           52048    52112      +64     
+ Misses         19847    19815      -32     
+ Partials        2650     2649       -1     

☔ 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 top, 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.

[leonardehrenfried]

I reworked the implementation according to our last discussion in the dev meeting and it's now ready for review again.

[t2gran]

The boolean argument is hard to guess/read. One way to get around it, is to use factory methods:

Make the constructor private and create two factory methods, maybe: Itinerary.createScheduledTransitItinerary(legs) and Itinerary.createDirectItinerary(legs). This will hide the flag searchWindowAware from the producers (they do not care) and make it an itinerary thing. The coupling between the producer and the flag also become clear and safe - there is less room for setting it wrong. If you want you can create a separate factory method for flex e.g. createTransitItinerary, createDirectFlexItinerary and createDirectStreetItinerary. The last two would have the same implementation, but they are easy to use - less room for mistakes.

[leonardehrenfried]

I've implemented this in 0538587.

The last suggestion of using separate methods for flex and street is hard without a larger refactoring because the two routers (A* and flex) use the same class to map from their internal representation to itineraries. I don't think it's worth it - do you, @t2gran?

[t2gran]

Looks good, some minor stuff left.