Skip to content

Commit d21da39

Browse files
vpaturetvpaturet
committed
Add FeedScopedId-keyed index to Timetable for O(1) TripTimes lookup
Timetable.getTripTimes(Trip) used == identity comparison, which fails when two different Trip instances share the same ID (e.g. after deserialization or in the shadow adapter). Replace the O(n) linear scan with an O(1) map lookup keyed by FeedScopedId, reusing the existing HashMap already maintained by TimetableBuilder.
1 parent 2be48ce commit d21da39

3 files changed

Lines changed: 88 additions & 33 deletions

File tree

‎application/src/main/java/org/opentripplanner/transit/model/timetable/Timetable.java‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import java.util.List;
99
import java.util.Map;
1010
import java.util.Optional;
11+
import java.util.stream.Collectors;
1112
import javax.annotation.Nullable;
1213
import org.opentripplanner.core.model.id.FeedScopedId;
1314
import org.opentripplanner.transit.model.network.TripPattern;
@@ -25,6 +26,8 @@ public class Timetable implements Serializable {
2526

2627
private List<TripTimes> tripTimes;
2728

29+
private Map<FeedScopedId, TripTimes> tripTimesIndex;
30+
2831
private List<FrequencyEntry> frequencyEntries;
2932

3033
@Nullable
@@ -36,6 +39,7 @@ public class Timetable implements Serializable {
3639
this.pattern = timetableBuilder.getPattern();
3740
this.serviceDate = timetableBuilder.getServiceDate();
3841
this.tripTimes = timetableBuilder.createImmutableOrderedListOfTripTimes();
42+
this.tripTimesIndex = timetableBuilder.createImmutableTripTimesIndex();
3943
this.frequencyEntries = List.copyOf(timetableBuilder.getFrequencies());
4044
this.maxTripSpanDays = computeMaxTripSpanDays(this.tripTimes);
4145
}
@@ -54,21 +58,12 @@ public TimetableBuilder copyOf() {
5458

5559
@Nullable
5660
public TripTimes getTripTimes(Trip trip) {
57-
for (TripTimes tt : tripTimes) {
58-
if (tt.getTrip() == trip) {
59-
return tt;
60-
}
61-
}
62-
return null;
61+
return getTripTimes(trip.getId());
6362
}
6463

64+
@Nullable
6565
public TripTimes getTripTimes(FeedScopedId tripId) {
66-
for (TripTimes tt : tripTimes) {
67-
if (tt.getTrip().getId().equals(tripId)) {
68-
return tt;
69-
}
70-
}
71-
return null;
66+
return tripTimesIndex.get(tripId);
7267
}
7368

7469
public boolean isValidFor(LocalDate serviceDate) {
@@ -82,6 +77,9 @@ public void setServiceCodes(Map<FeedScopedId, Integer> serviceCodes) {
8277
.stream()
8378
.map(tt -> tt.withServiceCode(serviceCodes.get(tt.getTrip().getServiceId())))
8479
.toList();
80+
tripTimesIndex = tripTimes
81+
.stream()
82+
.collect(Collectors.toUnmodifiableMap(tt -> tt.getTrip().getId(), tt -> tt));
8583
// Repeated code... bad sign...
8684
frequencyEntries = frequencyEntries
8785
.stream()

‎application/src/main/java/org/opentripplanner/transit/model/timetable/TimetableBuilder.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,10 @@ List<TripTimes> createImmutableOrderedListOfTripTimes() {
111111
return tripTimes.values().stream().sorted().toList();
112112
}
113113

114+
Map<FeedScopedId, TripTimes> createImmutableTripTimesIndex() {
115+
return Map.copyOf(tripTimes);
116+
}
117+
114118
TripPattern getPattern() {
115119
return pattern;
116120
}

‎application/src/test/java/org/opentripplanner/transit/model/timetable/TimetableTest.java‎

Lines changed: 74 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,23 @@
11
package org.opentripplanner.transit.model.timetable;
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertNull;
5+
import static org.junit.jupiter.api.Assertions.assertSame;
46
import static org.opentripplanner.transit.model._data.FeedScopedIdForTestFactory.id;
57

8+
import org.junit.jupiter.api.Test;
69
import org.junit.jupiter.params.ParameterizedTest;
710
import org.junit.jupiter.params.provider.CsvSource;
8-
import org.opentripplanner.transit.model._data.TimetableRepositoryForTest;
9-
import org.opentripplanner.transit.model.network.Route;
10-
import org.opentripplanner.transit.model.network.TripPattern;
11+
import org.opentripplanner.transit.model._data.TransitTestEnvironment;
12+
import org.opentripplanner.transit.model._data.TransitTestEnvironmentBuilder;
13+
import org.opentripplanner.transit.model._data.TripInput;
1114
import org.opentripplanner.transit.model.site.RegularStop;
1215

1316
class TimetableTest {
1417

15-
private static final TimetableRepositoryForTest TEST_MODEL = TimetableRepositoryForTest.of();
16-
17-
private static final Route ROUTE = TimetableRepositoryForTest.route("routeId").build();
18-
public static final RegularStop STOP_A = TEST_MODEL.stop("A").build();
19-
public static final RegularStop STOP_C = TEST_MODEL.stop("C").build();
18+
private final TransitTestEnvironmentBuilder envBuilder = TransitTestEnvironment.of();
19+
private final RegularStop stopA = envBuilder.stop("A");
20+
private final RegularStop stopC = envBuilder.stop("C");
2021

2122
@ParameterizedTest
2223
@CsvSource(
@@ -32,20 +33,72 @@ class TimetableTest {
3233
useHeadersInDisplayName = true
3334
)
3435
void maxTripSpanDays(String testCaseName, String schedule, int expectedNumberOfDays) {
35-
var timetable = TripPattern.of(id(testCaseName))
36-
.withRoute(ROUTE)
37-
.withStopPattern(TimetableRepositoryForTest.stopPattern(STOP_A, STOP_C))
38-
.withScheduledTimeTableBuilder(builder ->
39-
builder.addTripTimes(
40-
ScheduledTripTimes.of()
41-
.withTrip(TimetableRepositoryForTest.trip("t1").build())
42-
.withDepartureTimes(schedule)
43-
.build()
44-
)
45-
)
46-
.build()
47-
.getScheduledTimetable();
36+
var times = schedule.trim().split("\\s+");
37+
var env = envBuilder
38+
.addTrip(TripInput.of("t1").addStop(stopA, times[0]).addStop(stopC, times[1]))
39+
.build();
4840

41+
var timetable = env.tripData("t1").tripPattern().getScheduledTimetable();
4942
assertEquals(expectedNumberOfDays, timetable.getMaxTripSpanDays());
5043
}
44+
45+
@Test
46+
void getTripTimesByTrip() {
47+
var env = envBuilder
48+
.addTrip(TripInput.of("trip1").addStop(stopA, "08:00").addStop(stopC, "09:00"))
49+
.addTrip(TripInput.of("trip2").addStop(stopA, "10:00").addStop(stopC, "11:00"))
50+
.build();
51+
52+
var timetable = env.tripData("trip1").tripPattern().getScheduledTimetable();
53+
var trip1 = env.tripData("trip1").trip();
54+
var trip2 = env.tripData("trip2").trip();
55+
56+
assertSame(timetable.getTripTimes(trip1).getTrip(), trip1);
57+
assertSame(timetable.getTripTimes(trip2).getTrip(), trip2);
58+
}
59+
60+
@Test
61+
void getTripTimesByFeedScopedId() {
62+
var env = envBuilder
63+
.addTrip(TripInput.of("trip1").addStop(stopA, "08:00").addStop(stopC, "09:00"))
64+
.addTrip(TripInput.of("trip2").addStop(stopA, "10:00").addStop(stopC, "11:00"))
65+
.build();
66+
67+
var timetable = env.tripData("trip1").tripPattern().getScheduledTimetable();
68+
var trip1 = env.tripData("trip1").trip();
69+
var trip2 = env.tripData("trip2").trip();
70+
71+
assertSame(timetable.getTripTimes(trip1.getId()).getTrip(), trip1);
72+
assertSame(timetable.getTripTimes(trip2.getId()).getTrip(), trip2);
73+
}
74+
75+
@Test
76+
void getTripTimesUsesEqualsNotIdentity() {
77+
var env = envBuilder
78+
.addTrip(TripInput.of("sameId").addStop(stopA, "08:00").addStop(stopC, "09:00"))
79+
.build();
80+
81+
var timetable = env.tripData("sameId").tripPattern().getScheduledTimetable();
82+
var trip1 = env.tripData("sameId").trip();
83+
84+
// Create a different Trip instance with the same ID
85+
var trip2 = Trip.of(id("sameId")).withRoute(envBuilder.route("lookupRoute")).build();
86+
assertEquals(trip1.getId(), trip2.getId());
87+
88+
// Lookup using the second instance should find the TripTimes stored with the first
89+
assertSame(trip1, timetable.getTripTimes(trip2).getTrip());
90+
}
91+
92+
@Test
93+
void getTripTimesReturnsNullForUnknownTrip() {
94+
var env = envBuilder
95+
.addTrip(TripInput.of("trip1").addStop(stopA, "08:00").addStop(stopC, "09:00"))
96+
.build();
97+
98+
var timetable = env.tripData("trip1").tripPattern().getScheduledTimetable();
99+
100+
var unknownTrip = Trip.of(id("unknown")).withRoute(envBuilder.route("lookupRoute2")).build();
101+
assertNull(timetable.getTripTimes(unknownTrip));
102+
assertNull(timetable.getTripTimes(id("unknown")));
103+
}
51104
}

0 commit comments

Comments
 (0)