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 8 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 @@ -37,8 +37,8 @@ public Duration getDuration(String fieldName) {
return (Duration) get(fieldName, TokenType.DURATION);
}

public int getInt(String fieldName) {
return (int) get(fieldName, TokenType.INT);
public Integer getInt(String fieldName) {
return (Integer) get(fieldName, TokenType.INT);
}

public String getString(String fieldName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public TokenBuilder withDuration(String fieldName, Duration v) {
return with(fieldName, TokenType.DURATION, v);
}

public TokenBuilder withInt(String fieldName, int v) {
public TokenBuilder withInt(String fieldName, Integer v) {
return with(fieldName, TokenType.INT, v);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public String valueToString(@Nullable Object value) {
case BYTE -> Byte.toString((byte) value);
case DURATION -> DurationUtils.durationToStr((Duration) value);
case ENUM -> ((Enum<?>) value).name();
case INT -> Integer.toString((int) value);
case INT -> Integer.toString((Integer) value);
case STRING -> (String) value;
case TIME_INSTANT -> value.toString();
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package org.opentripplanner.model.plan.paging.cursor;

import java.util.OptionalInt;
import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResults;
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 NumItinerariesFilterResults numItinerariesFilterResults;
private final OptionalInt streetOnlyCost;

public DefaultPageCursorInput() {
this.numItinerariesFilterResults = null;
this.streetOnlyCost = OptionalInt.empty();
}

public DefaultPageCursorInput(Builder builder) {
this.numItinerariesFilterResults = builder.numItinerariesFilterResults();
this.streetOnlyCost = builder.streetOnlyCost();
}

public static DefaultPageCursorInput.Builder of() {
return new Builder(new DefaultPageCursorInput());
}

public DefaultPageCursorInput.Builder copyOf() {
return new Builder(this);
}

@Override
public NumItinerariesFilterResults numItinerariesFilterResults() {
return numItinerariesFilterResults;
}

@Override
public OptionalInt streetOnlyCost() {
return streetOnlyCost;
}

@Override
public String toString() {
return ToStringBuilder
.of(DefaultPageCursorInput.class)
.addObj("numItinerariesFilterResults", numItinerariesFilterResults)
.addObj("streetOnlyCost", streetOnlyCost)
.toString();
}

public static class Builder {

private NumItinerariesFilterResults numItinerariesFilterResults;
private OptionalInt streetOnlyCost;

public Builder(DefaultPageCursorInput original) {
this.numItinerariesFilterResults = original.numItinerariesFilterResults;
this.streetOnlyCost = original.streetOnlyCost;
}

public NumItinerariesFilterResults numItinerariesFilterResults() {
return numItinerariesFilterResults;
}

public Builder withNumItinerariesFilterResults(
NumItinerariesFilterResults numItinerariesFilterResults
) {
this.numItinerariesFilterResults = numItinerariesFilterResults;
return this;
}

public OptionalInt streetOnlyCost() {
return streetOnlyCost;
}

public Builder withStreetOnlyCost(OptionalInt streetOnlyCost) {
this.streetOnlyCost = streetOnlyCost;
return this;
}

public DefaultPageCursorInput build() {
return new DefaultPageCursorInput(this);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.time.Duration;
import java.time.Instant;
import java.util.OptionalInt;
import javax.annotation.Nullable;
import org.opentripplanner.model.plan.ItinerarySortKey;
import org.opentripplanner.model.plan.SortOrder;
Expand All @@ -24,11 +25,17 @@ public record PageCursor(
Instant earliestDepartureTime,
Instant latestArrivalTime,
Duration searchWindow,
@Nullable ItinerarySortKey itineraryPageCut
@Nullable ItinerarySortKey itineraryPageCut,
OptionalInt streetOnlyCost
) {
public boolean containsItineraryPageCut() {
return itineraryPageCut != null;
}

public boolean containsStreetOnlyCost() {
return streetOnlyCost.isPresent();
}

@Nullable
public String encode() {
return PageCursorSerializer.encode(this);
Expand Down Expand Up @@ -71,6 +78,7 @@ public String toString() {
.addDuration("searchWindow", searchWindow)
// This will only include the sort vector, not everything else in the itinerary
.addObjOp("itineraryPageCut", itineraryPageCut, ItinerarySortKey::keyAsString)
.addObj("streetOnlyCost", streetOnlyCost)
.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import java.time.Duration;
import java.time.Instant;
import java.util.OptionalInt;
import javax.annotation.Nullable;
import org.opentripplanner.model.plan.ItinerarySortKey;
import org.opentripplanner.model.plan.SortOrder;
Expand All @@ -20,7 +21,7 @@ public class PageCursorFactory {
private Duration currentSearchWindow = null;
private boolean wholeSwUsed = 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 @@ -50,18 +51,23 @@ public PageCursorFactory withOriginalSearch(
}

/**
* This adds the page cursor input to the factory. The cursor input can contain information about filtering results
* or the best street only cost.
* <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 (pageCursorInput.numItinerariesFilterResults() != null) {
this.wholeSwUsed = false;
this.itineraryPageCut = pageCursorInput.numItinerariesFilterResults().pageCut();
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't completely understand these changes. In what scenario is numItinerariesFilterResults null and why is wholeSwUsed set as false only if numItinerariesFilterResults is not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

numItinerariesFilterResults() can be null if no itineraries were removed, I added a comment

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 above comment also requires looking at the whole context of the filter

Copy link
Member

Choose a reason for hiding this comment

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

You may rename wholeSwUsed to wholeSearchWindowUsed, but it it is the case that the state of the wholeSearchWindowUsed can be derived from is pageCursorInput.numItinerariesFilterResults() == null, replacing it with a method is maybe better.

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 changed wholeSwUsed to wholeSearchWindowUsed, but did not create a method

Copy link
Member

Choose a reason for hiding this comment

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

I think this is also by default true. Usually we try to keep false as the default value. Also, the toString method uses a different name for this, perhaps we should use that for either the variable name or as the method name as @t2gran suggested.

return this;
}

Expand Down Expand Up @@ -120,26 +126,42 @@ private void createPageCursors() {
else {
if (currentPageType == NEXT_PAGE) {
prevEdt = edtBeforeNewSw();
nextEdt = pageCursorInput.earliestRemovedDeparture();
nextEdt = pageCursorInput.numItinerariesFilterResults().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
.numItinerariesFilterResults()
.latestRemovedDeparture()
.minus(newSearchWindow)
.plusSeconds(60);
nextEdt = edtAfterUsedSw();
}
}

OptionalInt streetOnlyCost = pageCursorInput.streetOnlyCost();
prevCursor =
new PageCursor(
PREVIOUS_PAGE,
sortOrder,
prevEdt,
currentLat,
newSearchWindow,
itineraryPageCut
itineraryPageCut,
streetOnlyCost
);
nextCursor =
new PageCursor(NEXT_PAGE, sortOrder, nextEdt, null, newSearchWindow, itineraryPageCut);
new PageCursor(
NEXT_PAGE,
sortOrder,
nextEdt,
null,
newSearchWindow,
itineraryPageCut,
streetOnlyCost
);
}

private Instant edtBeforeNewSw() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,29 +1,24 @@
package org.opentripplanner.model.plan.paging.cursor;

import java.time.Instant;
import org.opentripplanner.model.plan.ItinerarySortKey;
import java.util.OptionalInt;
import org.opentripplanner.routing.algorithm.filterchain.filters.system.NumItinerariesFilterResults;

/**
* 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.
* 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.
* <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.
* The streetOnlyCost is the cost of the best street only itinerary that was found in the first search.
*/
public interface PageCursorInput {
NumItinerariesFilterResults numItinerariesFilterResults();
/**
* 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 best street only cost 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.
*/
Instant earliestRemovedDeparture();
Instant latestRemovedDeparture();

/**
* 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.
*/
ItinerarySortKey pageCut();
OptionalInt streetOnlyCost();
}
Loading
Loading