Skip to content

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

Open
wants to merge 53 commits into
base: dev-2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
316db17
Initial version.
VillePihlava Feb 14, 2025
4391ac3
Fix null checks.
VillePihlava Feb 14, 2025
cd6a152
Fix tests.
VillePihlava Feb 14, 2025
2324f68
Fix tests and implementation.
VillePihlava Feb 18, 2025
5b1e251
Fix implementation.
VillePihlava Feb 20, 2025
77c3cfc
Add tests.
VillePihlava Feb 20, 2025
9af6665
Remove direct mode search with paging.
VillePihlava Feb 21, 2025
7c84581
Refactor bestStreetOnlyCost to streetOnlyCost, related to PageCursor.
VillePihlava Feb 21, 2025
ba3a817
Add RemoveTransitIfStreetOnlyIsBetterResults and refactoring.
VillePihlava Feb 27, 2025
97a4749
Make DefaultPageCursorInput constructors private.
VillePihlava Feb 27, 2025
e59b70a
Change comment.
VillePihlava Feb 27, 2025
45a7c43
Change comments.
VillePihlava Feb 27, 2025
7cabb40
Refactor how the DefaultPageCursorInput is built in RoutingWorker.
VillePihlava Feb 27, 2025
b66e83f
Add toString method to NumItinerariesFilterResults.
VillePihlava Feb 27, 2025
e7b641f
Refactor and document RemoveTransitIfStreetOnlyIsBetter, and throw er…
VillePihlava Feb 27, 2025
833acec
Add documentation.
VillePihlava Feb 27, 2025
56009b7
Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …
VillePihlava Mar 18, 2025
d46147f
Remove unnecessary changes.
VillePihlava Mar 18, 2025
6f42043
Remove uses of the IGNORE_SUBSCRIBER and throw error if null subsribe…
VillePihlava Mar 25, 2025
a4b6b3f
Change checks.
VillePihlava Mar 25, 2025
433de9f
Add initialization logic back to NumItinerariesFilterResults.
VillePihlava Mar 25, 2025
cd40ff6
Add comment to withPageCursorInput.
VillePihlava Mar 25, 2025
67a86c1
Remove comment.
VillePihlava Mar 25, 2025
fbcd0f4
Add back emptyDirectModeHandler with a case for when paging is used.
VillePihlava Mar 25, 2025
de777a3
Change comment.
VillePihlava Mar 25, 2025
81f5e89
Shorten link.
VillePihlava Mar 25, 2025
556a8f3
Add toString method.
VillePihlava Mar 25, 2025
da84174
Change comment.
VillePihlava Mar 25, 2025
10735b1
Refactor NumItinerariesFilterResults to NumItinerariesFilterResult.
VillePihlava Mar 25, 2025
1056e88
Refactor RemoveTransitIfStreetOnlyIsBetterResults to RemoveTransitIfS…
VillePihlava Mar 25, 2025
475e6e7
Fix format.
VillePihlava Mar 25, 2025
63723fe
Refactor DefaultPageCursorInput into org.opentripplanner.routing.algo…
VillePihlava Mar 27, 2025
4663c03
Rename wholeSwUsed to wholeSearchWindowUsed.
VillePihlava Mar 27, 2025
6c15475
Change comments.
VillePihlava Mar 27, 2025
49eb97b
Change generalizedCostMaxLimit logic in RemoveTransitIfStreetOnlyIsBe…
VillePihlava Mar 27, 2025
2b7b7e4
Add get method for numItinerariesFilterResult.
VillePihlava Mar 28, 2025
53c8e0f
Add get method for removeTransitIfStreetOnlyIsBetterResult.
VillePihlava Mar 28, 2025
643f69c
Add PageCursorInputAggregator.
VillePihlava Mar 28, 2025
61a3e55
Add PageCursorInputAggregator to ItineraryListFilterChain.
VillePihlava Mar 28, 2025
73d96c1
Add functionality for aggregator.
VillePihlava Mar 28, 2025
bfab7ce
Fix tests.
VillePihlava Mar 28, 2025
b2a541e
Change location of DefaultPageCursorInput.
VillePihlava Mar 28, 2025
3e99206
Remove unused imports.
VillePihlava Apr 1, 2025
d8219ec
Change createPageCursorInput to providePageCursorInput.
VillePihlava Apr 1, 2025
1df39de
Add back removed file FilterTransitWhenDirectModeIsEmptyTest.
VillePihlava Apr 1, 2025
92c68fc
Change documentation.
VillePihlava Apr 1, 2025
58fedb9
Clarify parameter in test.
VillePihlava Apr 1, 2025
a3801e2
Remove unnecessary code from tests.
VillePihlava Apr 1, 2025
cf9dde8
Change hasDeduplicationParams back to hasDedupeParams.
VillePihlava Apr 11, 2025
d830c17
Break comment lines at 100 characters.
VillePihlava Apr 11, 2025
545baa2
Change comments in NumItinerariesFilterResult.
VillePihlava Apr 15, 2025
12f389b
Add nullable annotations.
VillePihlava Apr 15, 2025
ce4b25d
Change comment.
VillePihlava Apr 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
import java.time.Duration;
import java.time.Instant;
import javax.annotation.Nullable;
import org.opentripplanner.framework.model.Cost;
import org.opentripplanner.model.plan.ItinerarySortKey;
import org.opentripplanner.model.plan.SortOrder;
import org.opentripplanner.routing.algorithm.filterchain.paging.DefaultPageCursorInput;
import org.opentripplanner.utils.tostring.ToStringBuilder;

public class PageCursorFactory {
Expand All @@ -18,9 +20,9 @@ public class PageCursorFactory {
private Instant currentEdt = null;
private Instant currentLat = null;
private Duration currentSearchWindow = null;
private boolean wholeSwUsed = true;
private boolean wholeSearchWindowUsed = true;
private ItinerarySortKey itineraryPageCut = null;
private PageCursorInput pageCursorInput = null;
private PageCursorInput pageCursorInput = DefaultPageCursorInput.of().build();

private PageCursor nextCursor = null;
private PageCursor prevCursor = null;
Expand Down Expand Up @@ -51,18 +53,23 @@ public PageCursorFactory withOriginalSearch(
}

/**
* This adds the page cursor input to the factory. The cursor input contains information about filtering results.
* <p>
* If there were itineraries removed in the current search because the numItineraries parameter
* was used, then we want to allow the caller to move within some of the itineraries that were
* removed in the next and previous pages. This means we will use information from when we cropped
* the list of itineraries to create the new search encoded in the page cursors. We will also add
* information necessary for removing potential duplicates when paging.
*
* @param pageCursorFactoryParams contains the result from the {@code PagingDuplicateFilter}
* @param pageCursorInput contains the generated page cursor input
*/
public PageCursorFactory withRemovedItineraries(PageCursorInput pageCursorFactoryParams) {
this.wholeSwUsed = false;
this.pageCursorInput = pageCursorFactoryParams;
this.itineraryPageCut = pageCursorFactoryParams.pageCut();
public PageCursorFactory withPageCursorInput(PageCursorInput pageCursorInput) {
this.pageCursorInput = pageCursorInput;
// If the whole search window was not used (i.e. if there were removed itineraries)
if (pageCursorInput.numItinerariesFilterResult() != null) {
this.wholeSearchWindowUsed = false;
this.itineraryPageCut = pageCursorInput.numItinerariesFilterResult().pageCut();
}
return this;
}

Expand All @@ -87,7 +94,7 @@ public String toString() {
.addDateTime("currentLat", currentLat)
.addDuration("currentSearchWindow", currentSearchWindow)
.addDuration("newSearchWindow", newSearchWindow)
.addBoolIfTrue("searchWindowCropped", !wholeSwUsed)
.addBoolIfTrue("searchWindowCropped", !wholeSearchWindowUsed)
.addObj("pageCursorFactoryParams", pageCursorInput)
.addObj("nextCursor", nextCursor)
.addObj("prevCursor", prevCursor)
Expand All @@ -112,42 +119,52 @@ private void createPageCursors() {
Instant prevEdt;
Instant nextEdt;

if (wholeSwUsed) {
if (wholeSearchWindowUsed) {
prevEdt = edtBeforeNewSw();
nextEdt = edtAfterUsedSw();
}
// If the whole search window was not used (i.e. if there were removed itineraries)
else {
if (currentPageType == NEXT_PAGE) {
prevEdt = edtBeforeNewSw();
nextEdt = pageCursorInput.earliestRemovedDeparture();
nextEdt = pageCursorInput.numItinerariesFilterResult().earliestRemovedDeparture();
} else {
// The search-window start and end is [inclusive, exclusive], so to calculate the start of the
// search-window from the last time included in the search window we need to include one extra
// minute at the end.
prevEdt = pageCursorInput.latestRemovedDeparture().minus(newSearchWindow).plusSeconds(60);
prevEdt = pageCursorInput
.numItinerariesFilterResult()
.latestRemovedDeparture()
.minus(newSearchWindow)
.plusSeconds(60);
nextEdt = edtAfterUsedSw();
}
}
// TODO - insert generalizedCostMaxLimit here

Cost generalizedCostMaxLimit = null;
if (pageCursorInput.removeTransitIfStreetOnlyIsBetterResult() != null) {
generalizedCostMaxLimit = pageCursorInput
.removeTransitIfStreetOnlyIsBetterResult()
.generalizedCostMaxLimit();
}

prevCursor = new PageCursor(
PREVIOUS_PAGE,
sortOrder,
prevEdt,
currentLat,
newSearchWindow,
itineraryPageCut,
null
generalizedCostMaxLimit
);
// TODO - insert generalizedCostMaxLimit here
nextCursor = new PageCursor(
NEXT_PAGE,
sortOrder,
nextEdt,
null,
newSearchWindow,
itineraryPageCut,
null
generalizedCostMaxLimit
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,29 +1,25 @@
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}
* 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.
* The itinerary filter chain may crop the search window as a result of reducing the list down to a specified maximum number of itineraries.
* This contains the results from {@link org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilter}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The o.o.model.plan.paging package should not have dependency on itinerary-filter-chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResult and org.opentripplanner.routing.algorithm.filterchain.filters.transit.RemoveTransitIfStreetOnlyIsBetterResult are used from the filterchain, should this be changed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we decide what we should do about this?

Copy link
Member

Choose a reason for hiding this comment

The 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.
* {@link RemoveTransitIfStreetOnlyIsBetterResult} contains a maximum cost for itineraries.
* It is calculated by 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
Expand Up @@ -6,14 +6,11 @@
import org.opentripplanner.model.plan.ItinerarySortKey;
import org.opentripplanner.model.plan.SortOrder;
import org.opentripplanner.utils.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

final class PageCursorSerializer {

private static final byte VERSION_ONE = 1;
private static final byte VERSION_TWO = 2;
private static final Logger LOG = LoggerFactory.getLogger(PageCursor.class);

private static final String TYPE_FIELD = "Type";
private static final String EDT_FIELD = "EDT";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ public class RoutingWorker {
private PageCursorInput pageCursorInput = null;

public RoutingWorker(OtpServerRequestContext serverContext, RouteRequest request, ZoneId zoneId) {
// Applying the page cursor modifies the request by removing the direct mode, for example.
// If the page cursor doesn't exist, this function does nothing.
request.applyPageCursor();
this.request = request;
this.serverContext = serverContext;
Expand All @@ -92,7 +94,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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,25 @@
import org.opentripplanner.routing.algorithm.filterchain.framework.filterchain.DeleteResultHandler;
import org.opentripplanner.routing.algorithm.filterchain.framework.filterchain.RoutingErrorsAttacher;
import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryListFilter;
import org.opentripplanner.routing.algorithm.filterchain.paging.PageCursorInputAggregator;
import org.opentripplanner.routing.api.response.RoutingError;

public class ItineraryListFilterChain {

private final List<ItineraryListFilter> filters;
private final DeleteResultHandler debugHandler;
private final PageCursorInputAggregator pageCursorInputAggregator;

private final List<RoutingError> routingErrors = new ArrayList<>();

public ItineraryListFilterChain(
List<ItineraryListFilter> filters,
DeleteResultHandler debugHandler
DeleteResultHandler debugHandler,
PageCursorInputAggregator pageCursorInputAggregator
) {
this.debugHandler = debugHandler;
this.filters = filters;
this.pageCursorInputAggregator = pageCursorInputAggregator;
}

public List<Itinerary> filter(List<Itinerary> itineraries) {
Expand All @@ -29,6 +33,8 @@ public List<Itinerary> filter(List<Itinerary> itineraries) {
result = filter.filter(result);
}

pageCursorInputAggregator.providePageCursorInput();

routingErrors.addAll(RoutingErrorsAttacher.computeErrors(itineraries, result));

return debugHandler.filter(result);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import javax.annotation.Nullable;
import org.opentripplanner.ext.accessibilityscore.DecorateWithAccessibilityScore;
import org.opentripplanner.framework.application.OTPFeature;
import org.opentripplanner.framework.model.Cost;
import org.opentripplanner.model.plan.Itinerary;
import org.opentripplanner.model.plan.ItinerarySortKey;
import org.opentripplanner.model.plan.SortOrder;
Expand Down Expand Up @@ -50,6 +51,7 @@
import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryDecorator;
import org.opentripplanner.routing.algorithm.filterchain.framework.spi.ItineraryListFilter;
import org.opentripplanner.routing.algorithm.filterchain.framework.spi.RemoveItineraryFlagger;
import org.opentripplanner.routing.algorithm.filterchain.paging.PageCursorInputAggregator;
import org.opentripplanner.routing.api.request.framework.CostLinearFunction;
import org.opentripplanner.routing.api.request.preference.ItineraryFilterDebugProfile;
import org.opentripplanner.routing.services.TransitAlertService;
Expand Down Expand Up @@ -78,7 +80,7 @@ public class ItineraryListFilterChainBuilder {
private double bikeRentalDistanceRatio;
private double parkAndRideDurationRatio;
private CostLinearFunction nonTransitGeneralizedCostLimit;
private Consumer<PageCursorInput> pageCursorInputSubscriber;
private Consumer<PageCursorInput> pageCursorInputSubscriber = i -> {};
private Instant earliestDepartureTime = null;
private Duration searchWindow = null;
private boolean accessibilityScore;
Expand All @@ -89,6 +91,7 @@ public class ItineraryListFilterChainBuilder {
private double minBikeParkingDistance;
private boolean removeTransitIfWalkingIsBetter = true;
private ItinerarySortKey itineraryPageCut;
private Cost generalizedCostMaxLimit = null;
private boolean transitGroupPriorityUsed = false;
private boolean filterDirectFlexBySearchWindow = true;

Expand Down Expand Up @@ -272,10 +275,8 @@ public ItineraryListFilterChainBuilder withSearchWindow(
}

/**
* If the maximum number of itineraries is exceeded, then the excess itineraries are removed.
* The paging service needs this information to adjust the paging cursor. The 'maxLimit' check is
* the last thing* happening in the filter-chain after the final sort. So, if another filter
* removes an itinerary, the itinerary is not considered with respect to this limit.
* Paging stores information from the original search for use with related requests.
* This subscriber stores the input needed for creating the page cursor.
*/
public ItineraryListFilterChainBuilder withPageCursorInputSubscriber(
Consumer<PageCursorInput> pageCursorInputSubscriber
Expand All @@ -284,6 +285,17 @@ public ItineraryListFilterChainBuilder withPageCursorInputSubscriber(
return this;
}

/**
* If the search is done with a page cursor that contains an encoded best street only cost, then
* this function adds the information to the {@link RemoveTransitIfStreetOnlyIsBetter} filter.
*
* @param generalizedCostMaxLimit the best street only cost used in filtering.
*/
public ItineraryListFilterChainBuilder withGeneralizedCostMaxLimit(Cost generalizedCostMaxLimit) {
this.generalizedCostMaxLimit = generalizedCostMaxLimit;
return this;
}

/**
* If the search is done with a page cursor that contains encoded deduplication parameters, then
* this function adds the filter that removes duplicates.
Expand Down Expand Up @@ -383,6 +395,8 @@ public ItineraryListFilterChainBuilder withTransitAlerts(
@SuppressWarnings("CollectionAddAllCanBeReplacedWithConstructor")
public ItineraryListFilterChain build() {
List<ItineraryListFilter> filters = new ArrayList<>();
NumItinerariesFilter numItinerariesFilter = null;
RemoveTransitIfStreetOnlyIsBetter removeTransitIfStreetOnlyIsBetter = null;

filters.addAll(buildGroupByTripIdAndDistanceFilters());

Expand Down Expand Up @@ -433,10 +447,11 @@ public ItineraryListFilterChain build() {
{
// Filter transit itineraries by comparing against non-transit using generalized-cost
if (removeTransitWithHigherCostThanBestOnStreetOnly != null) {
addRemoveFilter(
filters,
new RemoveTransitIfStreetOnlyIsBetter(removeTransitWithHigherCostThanBestOnStreetOnly)
removeTransitIfStreetOnlyIsBetter = new RemoveTransitIfStreetOnlyIsBetter(
removeTransitWithHigherCostThanBestOnStreetOnly,
generalizedCostMaxLimit
);
addRemoveFilter(filters, removeTransitIfStreetOnlyIsBetter);
}

if (removeTransitIfWalkingIsBetter) {
Expand Down Expand Up @@ -490,14 +505,11 @@ public ItineraryListFilterChain build() {
// Remove itineraries if max limit is set
if (maxNumberOfItineraries > 0) {
addSort(filters, SortOrderComparator.comparator(sortOrder));
addRemoveFilter(
filters,
new NumItinerariesFilter(
maxNumberOfItineraries,
maxNumberOfItinerariesCropSection,
pageCursorInputSubscriber
)
numItinerariesFilter = new NumItinerariesFilter(
maxNumberOfItineraries,
maxNumberOfItinerariesCropSection
);
addRemoveFilter(filters, numItinerariesFilter);
}
}

Expand Down Expand Up @@ -538,8 +550,13 @@ public ItineraryListFilterChain build() {
}

var debugHandler = new DeleteResultHandler(debug, maxNumberOfItineraries);
PageCursorInputAggregator pageCursorInputAggregator = PageCursorInputAggregator.of()
.withNumItinerariesFilter(numItinerariesFilter)
.withRemoveTransitIfStreetOnlyIsBetter(removeTransitIfStreetOnlyIsBetter)
.withPageCursorInputSubscriber(pageCursorInputSubscriber)
.build();

return new ItineraryListFilterChain(filters, debugHandler);
return new ItineraryListFilterChain(filters, debugHandler, pageCursorInputAggregator);
}

public ItineraryListFilterChainBuilder withFilterDirectFlexBySearchWindow(boolean b) {
Expand Down
Loading
Loading