Remove itineraries outside the search window in arriveBy search

[t2gran]

Summary

Paging does not work with arriveBy=true. The searchWindow latest-depature-time is not enforced, when paging. For more info, see issue.

Issue

Closes #5432

Unit tests

This PR updates the filter unit-tests, but I have not added an integration test for the itinerary.

Documentation

JavaDoc is updated, other doc is not changed.

Changelog

✅

Bumping the serialization version id

Not needed

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a9b0151) 66.80% compared to head (299520c) 66.80%.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5433      +/-   ##
=============================================
- Coverage      66.80%   66.80%   -0.01%     
- Complexity     15557    15563       +6     
=============================================
  Files           1806     1806              
  Lines          69821    69839      +18     
  Branches        7357     7359       +2     
=============================================
+ Hits           46646    46653       +7     
- Misses         20718    20729      +11     
  Partials        2457     2457              

Files	Coverage Δ
...m/filterchain/ItineraryListFilterChainBuilder.java	90.32% <100.00%> (+0.32%)	⬆️
...ain/deletionflagger/OutsideSearchWindowFilter.java	100.00% <100.00%> (ø)
...rithm/mapping/RouteRequestToFilterChainMapper.java	78.43% <100.00%> (ø)
...entripplanner/routing/algorithm/RoutingWorker.java	75.20% <89.47%> (-5.51%)	⬇️

... and 2 files with indirect coverage changes

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

[leonardehrenfried]

I've only noticed typos.

[jtorin]

I can confirm that we are affected by this issue as well.

I can also confirm that the PR resolves the problem except for the detail that the arrival time must be increased by one minute to include the same trip/result on the first search,

I have reviewed the PR but I am unable to spot a change in the code that could induce this .

Common info

Search parameters are as follows, only arrival time is changed:

Behavior before patching

Search on exact boundary, 6:05

“0 Itineraries Returned”

Search on boundary + 1, 6:06

Select “Previous”:

(same repeated four times)

Then:

Behavior after patching

Search on exact boundary, 6:05

“0 Itineraries Returned”, as before.

Search on boundary + 1, 6:06

“0 Itineraries Returned”, before trips was presented.

Search on boundary + 2, 6:07

Selecting “Previous” generates “0 Itineraries Returned” twice, then:

Conclusion

Duplicate entries are not presented any longer, but arrival time must be increased by one minute to give the same result as before.

[t2gran]

I can also confirm that the PR resolves the problem except for the detail that the arrival time must be increased by one minute to include the same trip/result on the first search

This is not an attempt to fix this. It only fixes the bug in the searchWindow itinerary filter. @eibakke is working on a fix to the paging logic.

[t2gran]

@jtorin I have updated the title of this PR - it was confusing.

[t2gran]

@jtorin I studied the test results again. There is not enough information to tell if the above is an error or not - I do not see that it is related to the thing I have fixed. The debug UI only show minutes, so when walking to the destination it is likely that you arrive there 06:05:01 witch would be to late if the dateTime is set to 06:05:00.

So, my question is - is the test you have above working on the current version of OTP?

There are another thing tat looks strange in the result as well. There is a very long wait time between the access walk and the transit - to me this is an indication that the search-window is incorrect calculated on the server.

[leonardehrenfried]

You need to resolve the conflicts.

[leonardehrenfried]

@Bartosz-Kruba will review.

[jtorin]

So, my question is - is the test you have above working on the current version of OTP?

Yes.

On dev:

(same commit as the latest merge)

(version: 2.5.0-SNAPSHOT, ser.ver.id: 123, commit: a9b0151, branch: a9b0151) using Java 21.0.1

Results comes first when searching from 6:06.

With PR:

(version: 2.5.0-SNAPSHOT, ser.ver.id: 123, commit: 299520c, branch: entur/otp2_fix_prev_page_arrive_by_true) using Java 21.0.1

Results comes first when searching from 6:07.

---

Otherwise same data and config.

[jtorin]

For completeness, here are links to the data used to build the graph used in my testing:

• https://jpsapubdev.blob.core.windows.net/resesok-pub/ST_netex_acc_2023-19-13.zip
• https://jpsapubdev.blob.core.windows.net/resesok-pub/sweden-filtered.osm.pbf
• ( https://jpsapubdev.blob.core.windows.net/resesok-pub/denmark-oresund.osm.pbf )

[t2gran]

Thank you @jtorin for the findings and the data. I suggest we merge this and look into the bug after #5436 and #5458 is merged. Our client team have reported other bugs on this as well and it is just not possible to test this properly without merging all these fixes. Currently we had to turn "arriveBy" off, because it does not work - so, the one missed trip is a minor thing compared to the other errors.

The only explanation I have for the behaviour you have reported is that in that there is something wrong with the heuristic. I find the result a bit odd because of the wait time after the access walk - this does not look right.

Another tip is to turn off the itinerary filter, then if the trip is removed by the search-window-filter it should say so - all trips are retuned and deleted trips are tagged with the filter(s) deleting it.

[jtorin]

Another tip is to turn off the itinerary filter, then if the trip is removed by the search-window-filter it should say so - all trips are retuned and deleted trips are tagged with the filter(s) deleting it.

Yes, as expected the missing trip is returned tagged 'outside-search-window'. Increasing the search-window drastically will eventually produce other trips from the previous day. I guess the search-window is only (and obviously) increased backwards in time, not that one second forwards that is required to find the suppressed trip.

Still, it's nevertheless interesting that the trip is found and filtered, shows that perhaps the search is correct but the filtering not.

[t2gran]

@jtorin I have tested the reported case with #5458 today. I promised to come back to this.

First - The exact arrival time is 6:05:26 am (1697688326000 EPOCH) (use the browser developer tools). So if you set the arrival-time to 06:05, then the expected result should be NOT FOUND.

Duplicate entries are not presented any longer, but arrival time must be increased by one minute to give the same result as before.

"but arrival time must be increased" this is a bug in the old version of OTP, as explained above - when setting the arrival time to 06:05:00 - you should not get a result.

[t2gran]

There is still a problem with the arriveBy search and paging. I am looking into this.