-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add generalizedCostMaxLimit
field to the PageCursor
to enable using RemoveTransitIfStreetOnlyIsBetter
filter with paging
#6474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev-2.x
Are you sure you want to change the base?
Changes from 31 commits
316db17
4391ac3
cd6a152
2324f68
5b1e251
77c3cfc
9af6665
7c84581
ba3a817
97a4749
e59b70a
45a7c43
7cabb40
b66e83f
e7b641f
833acec
56009b7
d46147f
6f42043
a4b6b3f
433de9f
cd40ff6
67a86c1
fbcd0f4
de777a3
81f5e89
556a8f3
da84174
10735b1
1056e88
475e6e7
63723fe
4663c03
6c15475
49eb97b
2b7b7e4
53c8e0f
643f69c
61a3e55
73d96c1
bfab7ce
b2a541e
3e99206
d8219ec
1df39de
92c68fc
58fedb9
a3801e2
cf9dde8
d830c17
545baa2
12f389b
ce4b25d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
package org.opentripplanner.model.plan.paging.cursor; | ||
|
||
import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResult; | ||
import org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetterResult; | ||
import org.opentripplanner.utils.tostring.ToStringBuilder; | ||
|
||
/** | ||
* This class stores input for the PageCursor. The input is related to the NumItinerariesFilter and RemoveTransitIfStreetOnlyIsBetter. | ||
* <p> | ||
* The NumItinerariesFilter removes itineraries from a list of itineraries based on the number to | ||
* keep and whether it should crop at the head or the tail of the list. This class keeps | ||
* the extreme endpoints of the sets of itineraries that were kept and removed, as well as more | ||
* details about the first itinerary removed (bottom of the head, or top of the tail) and whether | ||
* itineraries were cropped at the head or the tail. | ||
* <p> | ||
* The {@link org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetter} | ||
* filter removes transit itineraries if the best street only itinerary has a lower cost. | ||
* This class stores the cost of the best street only itinerary for use with paging. | ||
*/ | ||
public class DefaultPageCursorInput implements PageCursorInput { | ||
|
||
private final NumItinerariesFilterResult numItinerariesFilterResult; | ||
private final RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult; | ||
|
||
private DefaultPageCursorInput() { | ||
this.numItinerariesFilterResult = null; | ||
this.removeTransitIfStreetOnlyIsBetterResult = null; | ||
} | ||
|
||
private DefaultPageCursorInput(Builder builder) { | ||
this.numItinerariesFilterResult = builder.numItinerariesFilterResult(); | ||
this.removeTransitIfStreetOnlyIsBetterResult = | ||
builder.removeTransitIfStreetOnlyIsBetterResult(); | ||
} | ||
|
||
public static DefaultPageCursorInput.Builder of() { | ||
return new Builder(new DefaultPageCursorInput()); | ||
} | ||
|
||
public DefaultPageCursorInput.Builder copyOf() { | ||
return new Builder(this); | ||
} | ||
|
||
@Override | ||
public NumItinerariesFilterResult numItinerariesFilterResult() { | ||
return numItinerariesFilterResult; | ||
} | ||
|
||
@Override | ||
public RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult() { | ||
return removeTransitIfStreetOnlyIsBetterResult; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return ToStringBuilder.of(DefaultPageCursorInput.class) | ||
.addObj("numItinerariesFilterResult", numItinerariesFilterResult) | ||
.addObj("removeTransitIfStreetOnlyIsBetterResult", removeTransitIfStreetOnlyIsBetterResult) | ||
.toString(); | ||
} | ||
|
||
public static class Builder { | ||
|
||
private NumItinerariesFilterResult numItinerariesFilterResult; | ||
private RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult; | ||
|
||
public Builder(DefaultPageCursorInput original) { | ||
this.numItinerariesFilterResult = original.numItinerariesFilterResult; | ||
this.removeTransitIfStreetOnlyIsBetterResult = | ||
original.removeTransitIfStreetOnlyIsBetterResult; | ||
} | ||
|
||
public NumItinerariesFilterResult numItinerariesFilterResult() { | ||
return numItinerariesFilterResult; | ||
} | ||
|
||
public Builder withNumItinerariesFilterResult( | ||
NumItinerariesFilterResult numItinerariesFilterResult | ||
) { | ||
this.numItinerariesFilterResult = numItinerariesFilterResult; | ||
return this; | ||
} | ||
|
||
public RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult() { | ||
return removeTransitIfStreetOnlyIsBetterResult; | ||
} | ||
|
||
public Builder withRemoveTransitIfStreetOnlyIsBetterResult( | ||
RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult | ||
) { | ||
this.removeTransitIfStreetOnlyIsBetterResult = removeTransitIfStreetOnlyIsBetterResult; | ||
return this; | ||
} | ||
|
||
public DefaultPageCursorInput build() { | ||
return new DefaultPageCursorInput(this); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,23 @@ | ||
package org.opentripplanner.model.plan.paging.cursor; | ||
|
||
import java.time.Instant; | ||
import org.opentripplanner.model.plan.ItinerarySortKey; | ||
import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResult; | ||
import org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetterResult; | ||
|
||
/** | ||
* This class holds information needed to create the next/previous page cursors when there were | ||
* itineraries removed due to cropping the list of itineraries using the numItineraries parameter. | ||
* <p> | ||
* The Instant fields come from the sets of itineraries that were removed and the ones that were | ||
* kept as a result of using the numItineraries parameter. | ||
* This class holds information needed to create the next/previous page cursors either when there were | ||
* itineraries removed due to cropping the list of itineraries using the numItineraries parameter or | ||
* when the {@link org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetter} | ||
optionsome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* filter is used. | ||
*/ | ||
public interface PageCursorInput { | ||
/** | ||
* The earliest-removed-departure defines the start of the search-window following the | ||
* current window. To include this removed itinerary (and all other removed itineraries) | ||
* in the next-page search the search windows must overlap. | ||
* This contains the results from {@link org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilter}. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could not find any already used imports that were relevant here. ...Filter vs ...FilterResults There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we decide what we should do about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still haven't decided what to do about this? |
||
*/ | ||
Instant earliestRemovedDeparture(); | ||
Instant latestRemovedDeparture(); | ||
|
||
NumItinerariesFilterResult numItinerariesFilterResult(); | ||
/** | ||
* In case the result has too many results: The {@code numberOfItineraries} request parameter | ||
* is less than the number of itineraries found, then we keep the last itinerary kept and | ||
* returned as part of the result. The sort vector will be included in the page-cursor and | ||
* used in the next/previous page to filter away duplicates. | ||
* RemoveTransitIfStreetOnlyIsBetterResult contains the best street only cost that comes from taking the cost of the best street only itinerary from the first search. | ||
* This is used as a comparison in {@link org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetter} | ||
* when paging is used. | ||
*/ | ||
ItinerarySortKey pageCut(); | ||
RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,10 +12,12 @@ | |
import org.opentripplanner.framework.application.OTPRequestTimeoutException; | ||
import org.opentripplanner.model.plan.Itinerary; | ||
import org.opentripplanner.model.plan.grouppriority.TransitGroupPriorityItineraryDecorator; | ||
import org.opentripplanner.model.plan.paging.cursor.PageCursorInput; | ||
import org.opentripplanner.model.plan.paging.cursor.DefaultPageCursorInput; | ||
import org.opentripplanner.raptor.api.request.RaptorTuningParameters; | ||
import org.opentripplanner.raptor.api.request.SearchParams; | ||
import org.opentripplanner.routing.algorithm.filterchain.ItineraryListFilterChain; | ||
import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResult; | ||
import org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetterResult; | ||
import org.opentripplanner.routing.algorithm.mapping.PagingServiceFactory; | ||
import org.opentripplanner.routing.algorithm.mapping.RouteRequestToFilterChainMapper; | ||
import org.opentripplanner.routing.algorithm.mapping.RoutingResponseMapper; | ||
|
@@ -63,9 +65,12 @@ public class RoutingWorker { | |
private final AdditionalSearchDays additionalSearchDays; | ||
private final TransitGroupPriorityService transitGroupPriorityService; | ||
private SearchParams raptorSearchParamsUsed = null; | ||
private PageCursorInput pageCursorInput = null; | ||
private NumItinerariesFilterResult numItinerariesFilterResult = null; | ||
private RemoveTransitIfStreetOnlyIsBetterResult removeTransitIfStreetOnlyIsBetterResult = null; | ||
|
||
public RoutingWorker(OtpServerRequestContext serverContext, RouteRequest request, ZoneId zoneId) { | ||
// Applying the page cursor modifies the request by removing the direct mode, for example. | ||
optionsome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// If the page cursor doesn't exist, this function does nothing. | ||
request.applyPageCursor(); | ||
this.request = request; | ||
this.serverContext = serverContext; | ||
|
@@ -92,7 +97,8 @@ public RoutingResponse route() { | |
// If no direct mode is set, then we set one. | ||
// See {@link FilterTransitWhenDirectModeIsEmpty} | ||
var emptyDirectModeHandler = new FilterTransitWhenDirectModeIsEmpty( | ||
request.journey().direct().mode() | ||
request.journey().direct().mode(), | ||
request.pageCursor() != null | ||
); | ||
|
||
request.journey().direct().setMode(emptyDirectModeHandler.resolveDirectMode()); | ||
|
@@ -136,7 +142,8 @@ public RoutingResponse route() { | |
searchWindowUsed(), | ||
emptyDirectModeHandler.removeWalkAllTheWayResults() || | ||
removeWalkAllTheWayResultsFromDirectFlex, | ||
it -> pageCursorInput = it | ||
it -> numItinerariesFilterResult = it, | ||
it -> removeTransitIfStreetOnlyIsBetterResult = it | ||
); | ||
|
||
result.transform(filterChain::filter); | ||
|
@@ -280,7 +287,10 @@ private PagingService createPagingService(List<Itinerary> itineraries) { | |
serverContext.raptorTuningParameters(), | ||
request, | ||
raptorSearchParamsUsed, | ||
pageCursorInput, | ||
DefaultPageCursorInput.of() | ||
.withNumItinerariesFilterResult(numItinerariesFilterResult) | ||
.withRemoveTransitIfStreetOnlyIsBetterResult(removeTransitIfStreetOnlyIsBetterResult) | ||
.build(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the itinerary-filter-chain should be responsible for producing the PageCursorInput - this adds an extra layer of abstraction, but the coupling at field/information level is so high, that I do not se the point of doing it. The responsibility of the RoutingWorker is to wire things together, not mapping from one domain to another. The mapping logic should then be extracted, but it is simpler to just push it into he filter-chain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, we need the result of filters to produce the event to send back to the paging. There are many ways to do it: Alt 1. We can store the result in the filters and add the filters to a result aggregator. When the filter-chain is compleate, we notify the aggregator and it produces the paging event. Alt 2. The filters can send a event to the result aggregator and the aggregator can send an event to the paging when it has rescieved an event from both of the filters. This is complex, and if one of the filters is disabled it might fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created a PageCursorInputAggregator and use a callback to set the PageCursorInput |
||
itineraries | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PageCursorInput define a role implemented by the itinerary filter chain - we did this to invert the dependencies and to decouple the paging from the itinerary filter chain. There is no point in having the implementation of an interface in the same package (unless its just the default of many) - it indicates that there is a design mistake.
There should NOT be any dependencies from paging to the itinerary-filter-chain.
So, this should be moved inside the filter-chain, witch should be responsible for produsing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the
DefaultPageCursorInput
into theorg.opentripplanner.routing.algorithm.filterchain
packageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
PageCursorInput
needs to be reverted to f4bfedf, then add thegeneralizedCostMaxLimit
to it. This is the data the cursor factory needs to produce a cursor.Then move the
DefaultPageCursorInput
into the itinerary filter chain module:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why the separate result object exists is because it can be null and there is logic that checks whether it is null.
It worked before because the null check could be done on the pagecursorinput itself.
It can be null because of this:
https://github.com/HSLdevcom/OpenTripPlanner/blob/ce4b25da09b29f4ee436f65e3acd2a8a9090e21e/application/src/main/java/org/opentripplanner/routing/algorithm/filterchain/filters/system/NumItinerariesFilter.java#L34-L36
The null checks happen at least here:
https://github.com/HSLdevcom/OpenTripPlanner/blob/ce4b25da09b29f4ee436f65e3acd2a8a9090e21e/application/src/main/java/org/opentripplanner/model/plan/paging/cursor/PageCursorFactory.java#L69-L72
https://github.com/HSLdevcom/OpenTripPlanner/blob/ce4b25da09b29f4ee436f65e3acd2a8a9090e21e/application/src/main/java/org/opentripplanner/service/paging/PagingService.java#L100
https://github.com/HSLdevcom/OpenTripPlanner/blob/ce4b25da09b29f4ee436f65e3acd2a8a9090e21e/application/src/main/java/org/opentripplanner/service/paging/PagingService.java#L130
https://github.com/HSLdevcom/OpenTripPlanner/blob/ce4b25da09b29f4ee436f65e3acd2a8a9090e21e/application/src/main/java/org/opentripplanner/service/paging/PagingService.java#L136
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change the results objects to interfaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should I do a null check on the pageCut, for example?