From 263ee5aa0f53a6d315fd4fcf334e43db2d9ac9ed Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Wed, 13 Mar 2019 18:04:35 -0400 Subject: [PATCH 1/6] fix(validate): re-enable overlapping trips check fixes #207 --- .../gtfs/validator/NewTripTimesValidator.java | 13 +-- .../validator/OverlappingTripValidator.java | 85 +++++++++---------- .../gtfs/validator/ServiceValidator.java | 20 ++--- .../gtfs/validator/ValidationResult.java | 3 + src/test/java/com/conveyal/gtfs/GTFSTest.java | 82 +++++++----------- .../gtfs/storage/ErrorExpectation.java | 7 ++ .../gtfs/storage/PersistenceExpectation.java | 7 +- .../fake-agency-overlapping-trips/agency.txt | 2 + .../calendar.txt | 2 + .../calendar_dates.txt | 1 + .../fake-agency-overlapping-trips/routes.txt | 2 + .../stop_times.txt | 5 ++ .../fake-agency-overlapping-trips/stops.txt | 3 + .../fake-agency-overlapping-trips/trips.txt | 3 + 14 files changed, 118 insertions(+), 117 deletions(-) create mode 100644 src/test/java/com/conveyal/gtfs/storage/ErrorExpectation.java create mode 100755 src/test/resources/fake-agency-overlapping-trips/agency.txt create mode 100755 src/test/resources/fake-agency-overlapping-trips/calendar.txt create mode 100755 src/test/resources/fake-agency-overlapping-trips/calendar_dates.txt create mode 100755 src/test/resources/fake-agency-overlapping-trips/routes.txt create mode 100755 src/test/resources/fake-agency-overlapping-trips/stop_times.txt create mode 100755 src/test/resources/fake-agency-overlapping-trips/stops.txt create mode 100755 src/test/resources/fake-agency-overlapping-trips/trips.txt diff --git a/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java b/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java index c50cf929f..3d14e1541 100644 --- a/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java @@ -46,9 +46,11 @@ public NewTripTimesValidator(Feed feed, SQLErrorStorage errorStorage) { tripValidators = new TripValidator[] { new SpeedTripValidator(feed, errorStorage), new ReferencesTripValidator(feed, errorStorage), - //new OverlappingTripValidator(feed, errorStorage), new ReversedTripValidator(feed, errorStorage), new ServiceValidator(feed, errorStorage), + // Overlapping trips validator requires use of service info computed by ServiceValidator, so it must run + // after the ServiceValidator. + new OverlappingTripValidator(feed, errorStorage), new PatternFinderValidator(feed, errorStorage) }; } @@ -164,11 +166,10 @@ private void processTrip (List stopTimes) { // Repair the case where an arrival or departure time is provided, but not both. for (StopTime stopTime : stopTimes) fixMissingTimes(stopTime); // TODO check characteristics of timepoints - // All bad references should have been recorded at import, we can just ignore nulls. - Route route = null; - if (trip != null) route = routeById.get(trip.route_id); + // All bad references should have been recorded at import and null trip check is handled above, we can just + // ignore nulls. + Route route = routeById.get(trip.route_id); // Pass these same cleaned lists of stop_times and stops into each trip validator in turn. - for (TripValidator tripValidator : tripValidators) tripValidator.validateTrip(trip, route, stopTimes, stops); } @@ -177,7 +178,9 @@ private void processTrip (List stopTimes) { */ public void complete (ValidationResult validationResult) { for (TripValidator tripValidator : tripValidators) { + LOG.info("Running complete stage for {}", tripValidator.getClass().getSimpleName()); tripValidator.complete(validationResult); + LOG.info("{} finished", tripValidator.getClass().getSimpleName()); } } diff --git a/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java index c3603be72..422158aff 100644 --- a/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java @@ -1,30 +1,31 @@ package com.conveyal.gtfs.validator; -import com.conveyal.gtfs.error.NewGTFSError; -import com.conveyal.gtfs.error.OverlappingTripsInBlockError; import com.conveyal.gtfs.error.SQLErrorStorage; import com.conveyal.gtfs.loader.Feed; -import com.conveyal.gtfs.model.*; -import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; +import com.conveyal.gtfs.model.Route; +import com.conveyal.gtfs.model.Stop; +import com.conveyal.gtfs.model.StopTime; +import com.conveyal.gtfs.model.Trip; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.time.LocalDate; -import java.util.*; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Set; import static com.conveyal.gtfs.error.NewGTFSErrorType.TRIP_OVERLAP_IN_BLOCK; /** - * REVIEW - * whoa: feed.trips.values().stream().iterator().forEachRemaining(trip -> {}) - * should be for (Trip trip : feed.trips) {} - * We're fetching all the stop times for every trip in at least three different validators. - * * Created by landon on 5/2/16. */ public class OverlappingTripValidator extends TripValidator { - + private static final Logger LOG = LoggerFactory.getLogger(OverlappingTripValidator.class); // check for overlapping trips within block - HashMap> blockIntervals = new HashMap<>(); + private HashMap> blockIntervals = new HashMap<>(); public OverlappingTripValidator(Feed feed, SQLErrorStorage errorStorage) { super(feed, errorStorage); @@ -33,60 +34,50 @@ public OverlappingTripValidator(Feed feed, SQLErrorStorage errorStorage) { @Override public void validateTrip(Trip trip, Route route, List stopTimes, List stops) { if (trip.block_id != null) { + // If the trip has a block_id, add a new block interval to the BlockInterval blockInterval = new BlockInterval(); blockInterval.trip = trip; StopTime firstStopTime = stopTimes.get(0); blockInterval.startTime = firstStopTime.departure_time; blockInterval.firstStop = firstStopTime; blockInterval.lastStop = stopTimes.get(stopTimes.size() - 1); - List intervals = blockIntervals.get(trip.block_id); - if (intervals == null) { - intervals = new ArrayList<>(); - blockIntervals.put(trip.block_id, intervals); - } - intervals.add(blockInterval); + // Construct new list of intervals if none exists for encountered block_id. + blockIntervals + .computeIfAbsent(trip.block_id, k -> new ArrayList<>()) + .add(blockInterval); } } @Override public void complete (ValidationResult validationResult) { + // Iterate over each block and determine if there are any trips that overlap one another. for (String blockId : blockIntervals.keySet()) { List intervals = blockIntervals.get(blockId); - Collections.sort(intervals, Comparator.comparingInt(i -> i.startTime)); - int i2offset = 0; + intervals.sort(Comparator.comparingInt(i -> i.startTime)); + // Iterate over each interval (except for the last) comparing it to every other interval (so the last interval + // is handled through the course of iteration). // FIXME this has complexity of n^2, there has to be a better way. - for (BlockInterval i1 : intervals) { + for (int n = 0; n < intervals.size() - 1; n++) { + BlockInterval interval1 = intervals.get(n); // Compare the interval at position N with all other intervals at position N+1 to the end of the list. - i2offset += 1; // Fixme replace with integer iteration and get(n) - for(BlockInterval i2 : intervals.subList(i2offset, intervals.size() - 1)) { - String tripId1 = i1.trip.trip_id; - String tripId2 = i2.trip.trip_id; - if (tripId1.equals(tripId2)) { - // Why would this happen? Just duplicating a case covered by the original implementation. - // TODO can this happen at all? - continue; - } - // if trips don't overlap, skip FIXME can't this be simplified? - if (i1.lastStop.departure_time <= i2.firstStop.arrival_time || i2.lastStop.departure_time <= i1.firstStop.arrival_time) { + for (BlockInterval interval2 : intervals.subList(n + 1, intervals.size())) { + if (interval1.lastStop.departure_time <= interval2.firstStop.arrival_time || interval2.lastStop.departure_time <= interval1.firstStop.arrival_time) { continue; } - if (i1.trip.service_id.equals(i2.trip.service_id)) { - // Trips overlap. If they have the same service_id they overlap. - registerError(i1.trip, TRIP_OVERLAP_IN_BLOCK, i2.trip.trip_id); + // If either trip's last departure occurs after the other's first arrival, they overlap. We still + // need to determine if they operate on the same day though. + if (interval1.trip.service_id.equals(interval2.trip.service_id)) { + // If the overlapping trips share a service_id, record an error. + registerError(interval1.trip, TRIP_OVERLAP_IN_BLOCK, interval2.trip.trip_id); } else { // Trips overlap but don't have the same service_id. // Check to see if service days fall on the same days of the week. - // FIXME requires more complex Service implementation - Service s1 = null; //feed.services.get(i1.trip.service_id); - Service s2 = null; //feed.services.get(i2.trip.service_id); - boolean overlap = Service.checkOverlap(s1, s2); - for (Map.Entry d1 : s1.calendar_dates.entrySet()) { - LocalDate date = d1.getKey(); - boolean activeOnDate = s1.activeOn(date) && s2.activeOn(date); - if (activeOnDate || overlap) { // FIXME this will always be true if overlap is true. - registerError(i1.trip, TRIP_OVERLAP_IN_BLOCK, i2.trip.trip_id); - break; - } + ServiceValidator.ServiceInfo info1 = validationResult.serviceInfoForServiceId.get(interval1.trip.service_id); + ServiceValidator.ServiceInfo info2 = validationResult.serviceInfoForServiceId.get(interval2.trip.service_id); + Set overlappingDates = new HashSet<>(info1.datesActive); // use the copy constructor + overlappingDates.retainAll(info2.datesActive); + if (overlappingDates.size() > 0) { + registerError(interval1.trip, TRIP_OVERLAP_IN_BLOCK, interval2.trip.trip_id); } } } diff --git a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java index 3b1639c26..075015980 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java @@ -95,7 +95,7 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< */ @Override public void complete(ValidationResult validationResult) { - + validationResult.serviceInfoForServiceId = serviceInfoForServiceId; LOG.info("Merging calendars and calendar_dates..."); // First handle the calendar entries, which define repeating weekly schedules. @@ -113,7 +113,7 @@ public void complete(ValidationResult validationResult) { (dayOfWeek == DayOfWeek.SATURDAY && calendar.saturday > 0) || (dayOfWeek == DayOfWeek.SUNDAY && calendar.sunday > 0)) { // Service is active on this date. - serviceInfoForServiceId.computeIfAbsent(calendar.service_id, ServiceInfo::new).datesActive.add(date); + validationResult.serviceInfoForServiceId.computeIfAbsent(calendar.service_id, ServiceInfo::new).datesActive.add(date); } } } catch (Exception ex) { @@ -124,7 +124,7 @@ public void complete(ValidationResult validationResult) { // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. for (CalendarDate calendarDate : feed.calendarDates) { - ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); + ServiceInfo serviceInfo = validationResult.serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); if (calendarDate.exception_type == 1) { // Service added, add to set for this date. serviceInfo.datesActive.add(calendarDate.date); @@ -149,7 +149,7 @@ select durations.service_id, duration_seconds, days_active from ( // Check for incoherent or erroneous services. - for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { if (serviceInfo.datesActive.isEmpty()) { // This service must have been referenced by trips but is never active on any day. registerError(NewGTFSError.forFeed(NewGTFSErrorType.SERVICE_NEVER_ACTIVE, serviceInfo.serviceId)); @@ -166,7 +166,7 @@ select durations.service_id, duration_seconds, days_active from ( } // Accumulate info about services into each date that they are active. - for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { for (LocalDate date : serviceInfo.datesActive) { dateInfoForDate.computeIfAbsent(date, DateInfo::new).add(serviceInfo); } @@ -248,7 +248,7 @@ select durations.service_id, duration_seconds, days_active from ( sql = String.format("insert into %s values (?, ?, ?, ?)", servicesTableName); PreparedStatement serviceStatement = connection.prepareStatement(sql); final BatchTracker serviceTracker = new BatchTracker("services", serviceStatement); - for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { serviceStatement.setString(1, serviceInfo.serviceId); serviceStatement.setInt(2, serviceInfo.datesActive.size()); serviceStatement.setInt(3, serviceInfo.getTotalServiceDurationSeconds()); @@ -265,7 +265,7 @@ select durations.service_id, duration_seconds, days_active from ( sql = String.format("insert into %s values (?, ?)", serviceDatesTableName); PreparedStatement serviceDateStatement = connection.prepareStatement(sql); final BatchTracker serviceDateTracker = new BatchTracker("service_dates", serviceDateStatement); - for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { for (LocalDate date : serviceInfo.datesActive) { if (date == null) continue; // TODO ERR? Can happen with bad data (unparseable dates). try { @@ -302,7 +302,7 @@ select durations.service_id, duration_seconds, days_active from ( "service_durations", serviceDurationStatement ); - for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { serviceInfo.durationByRouteType.forEachEntry((routeType, serviceDurationSeconds) -> { try { serviceDurationStatement.setString(1, serviceInfo.serviceId); @@ -328,7 +328,7 @@ select durations.service_id, duration_seconds, days_active from ( LOG.info("Done."); } - private static class ServiceInfo { + static class ServiceInfo { final String serviceId; TIntIntHashMap durationByRouteType = new TIntIntHashMap(); @@ -345,7 +345,7 @@ public int getTotalServiceDurationSeconds() { } - private static class DateInfo { + static class DateInfo { final LocalDate date; TIntIntHashMap durationByRouteType = new TIntIntHashMap(); diff --git a/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java b/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java index d79a52444..aa65bd38c 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java +++ b/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java @@ -5,6 +5,8 @@ import java.awt.geom.Rectangle2D; import java.io.Serializable; import java.time.LocalDate; +import java.util.HashMap; +import java.util.Map; /** * An instance of this class is returned by the validator. @@ -38,6 +40,7 @@ public class ValidationResult implements Serializable { public int[] dailyRailSeconds; public int[] dailyTotalSeconds; public int[] dailyTripCounts; + public Map serviceInfoForServiceId = new HashMap<>(); public GeographicBounds fullBounds = new GeographicBounds(); public GeographicBounds boundsWithoutOutliers = new GeographicBounds(); public long validationTime; diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 3e2c8fe69..789bc940a 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -4,6 +4,7 @@ import com.conveyal.gtfs.error.NewGTFSErrorType; import com.conveyal.gtfs.loader.FeedLoadResult; import com.conveyal.gtfs.loader.SnapshotResult; +import com.conveyal.gtfs.storage.ErrorExpectation; import com.conveyal.gtfs.storage.ExpectedFieldType; import com.conveyal.gtfs.storage.PersistenceExpectation; import com.conveyal.gtfs.storage.RecordExpectation; @@ -125,6 +126,25 @@ public void canLoadFeedWithBadDates () { ); } + /** + * Tests that a GTFS feed with bad date values in calendars.txt and calendar_dates.txt can pass the integration test. + */ + @Test + public void canLoadFeedWithOverlappingTrips () { + PersistenceExpectation[] expectations = PersistenceExpectation.list( + new PersistenceExpectation( + new ErrorExpectation[]{ + new ErrorExpectation("error_type", "TRIP_OVERLAP_IN_BLOCK") + } + ) + ); + assertThat( + "Integration test passes", + runIntegrationTestOnFolder("fake-agency-overlapping-trips", nullValue(), expectations), + equalTo(true) + ); + } + /** * Tests whether or not "fake-agency" GTFS can be placed in a zipped subdirectory and loaded/exported successfully. */ @@ -231,6 +251,7 @@ private boolean runIntegrationTestOnFolder( Matcher fatalExceptionExpectation, PersistenceExpectation[] persistenceExpectations ) { + LOG.info("Running integration test on folder {}", folderName); // zip up test folder into temp zip file String zipFileName = null; try { @@ -267,7 +288,7 @@ private boolean runIntegrationTestOnZipFile( // Verify that loading the feed completes and data is stored properly try { // load and validate feed - LOG.info("load and validate feed"); + LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource); @@ -339,38 +360,6 @@ private void assertThatSnapshotIsErrorFree(SnapshotResult snapshotResult) { assertThat(snapshotResult.scheduleExceptions.fatalException, is(nullValue())); } - private String zipFolderAndLoadGTFS(String folderName, DataSource dataSource, PersistenceExpectation[] persistenceExpectations) { - // zip up test folder into temp zip file - String zipFileName; - try { - zipFileName = TestUtils.zipFolderFiles(folderName, true); - } catch (IOException e) { - e.printStackTrace(); - return null; - } - String namespace; - // Verify that loading the feed completes and data is stored properly - try { - // load and validate feed - LOG.info("load and validate feed"); - FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource); - DataSource ds = GTFS.createDataSource( - dataSource.getConnection().getMetaData().getURL(), - null, - null - ); - assertThatLoadIsErrorFree(loadResult); - assertThat(validationResult.fatalException, is(nullValue())); - namespace = loadResult.uniqueIdentifier; - assertThatImportedGtfsMeetsExpectations(ds.getConnection(), namespace, persistenceExpectations); - } catch (SQLException | AssertionError e) { - e.printStackTrace(); - return null; - } - return namespace; - } - /** * Helper function to export a GTFS from the database to a temporary zip file. */ @@ -421,28 +410,20 @@ private void assertThatImportedGtfsMeetsExpectations( int numRecordsSearched = 0; while (rs.next()) { numRecordsSearched++; - LOG.info(String.format("record %d in ResultSet", numRecordsSearched)); + LOG.info("record {} in ResultSet", numRecordsSearched); boolean allFieldsMatch = true; for (RecordExpectation recordExpectation: persistenceExpectation.recordExpectations) { switch (recordExpectation.expectedFieldType) { case DOUBLE: double doubleVal = rs.getDouble(recordExpectation.fieldName); - LOG.info(String.format( - "%s: %f", - recordExpectation.fieldName, - doubleVal - )); + LOG.info("{}: {}", recordExpectation.fieldName, doubleVal); if (doubleVal != recordExpectation.doubleExpectation) { allFieldsMatch = false; } break; case INT: int intVal = rs.getInt(recordExpectation.fieldName); - LOG.info(String.format( - "%s: %d", - recordExpectation.fieldName, - intVal - )); + LOG.info("{}: {}", recordExpectation.fieldName, intVal); if (intVal != recordExpectation.intExpectation) { fieldsWithMismatches.put( recordExpectation.fieldName, @@ -453,17 +434,10 @@ private void assertThatImportedGtfsMeetsExpectations( break; case STRING: String strVal = rs.getString(recordExpectation.fieldName); - LOG.info(String.format( - "%s: %s", - recordExpectation.fieldName, - strVal - )); + LOG.info("{}: {}", recordExpectation.fieldName, strVal); if (strVal == null && recordExpectation.stringExpectation == null) { break; - } else if ( - (strVal == null && recordExpectation.stringExpectation != null) || - !strVal.equals(recordExpectation.stringExpectation) - ) { + } else if (strVal == null || !strVal.equals(recordExpectation.stringExpectation)) { fieldsWithMismatches.put( recordExpectation.fieldName, new ValuePair(recordExpectation.stringExpectation, strVal) @@ -521,6 +495,8 @@ private void assertThatExportedGtfsMeetsExpectations( // iterate through all expectations for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { + // No need to check that errors were exported because it is an internal table only. + if ("errors".equals(persistenceExpectation.tableName)) continue; final String tableFileName = persistenceExpectation.tableName + ".txt"; LOG.info(String.format("reading table: %s", tableFileName)); diff --git a/src/test/java/com/conveyal/gtfs/storage/ErrorExpectation.java b/src/test/java/com/conveyal/gtfs/storage/ErrorExpectation.java new file mode 100644 index 000000000..d4ad653ff --- /dev/null +++ b/src/test/java/com/conveyal/gtfs/storage/ErrorExpectation.java @@ -0,0 +1,7 @@ +package com.conveyal.gtfs.storage; + +public class ErrorExpectation extends RecordExpectation { + public ErrorExpectation(String fieldName, String stringExpectation) { + super(fieldName, stringExpectation); + } +} diff --git a/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java b/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java index 1c1c1be7d..60bdc80a9 100644 --- a/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java +++ b/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java @@ -1,7 +1,5 @@ package com.conveyal.gtfs.storage; -import com.conveyal.gtfs.GTFSTest; - /** * A helper class to verify that data got stored in a particular table. */ @@ -20,6 +18,11 @@ public PersistenceExpectation(String tableName, RecordExpectation[] recordExpect this.recordExpectations = recordExpectations; } + public PersistenceExpectation(ErrorExpectation[] errorExpectations) { + this.tableName = "errors"; + this.recordExpectations = errorExpectations; + } + public static PersistenceExpectation[] list (PersistenceExpectation... expectations) { return expectations; } diff --git a/src/test/resources/fake-agency-overlapping-trips/agency.txt b/src/test/resources/fake-agency-overlapping-trips/agency.txt new file mode 100755 index 000000000..fc4cc2732 --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/agency.txt @@ -0,0 +1,2 @@ +agency_id,agency_name,agency_url,agency_lang,agency_phone,agency_email,agency_timezone,agency_fare_url +100000000,Fake Transit,http://example.com,,,,America/Los_Angeles, diff --git a/src/test/resources/fake-agency-overlapping-trips/calendar.txt b/src/test/resources/fake-agency-overlapping-trips/calendar.txt new file mode 100755 index 000000000..3959f46da --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/calendar.txt @@ -0,0 +1,2 @@ +service_id,monday,tuesday,wednesday,thursday,friday,saturday,sunday,start_date,end_date +A,1,1,1,1,1,1,1,20190313,20190917 diff --git a/src/test/resources/fake-agency-overlapping-trips/calendar_dates.txt b/src/test/resources/fake-agency-overlapping-trips/calendar_dates.txt new file mode 100755 index 000000000..74c1ef632 --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/calendar_dates.txt @@ -0,0 +1 @@ +service_id,date,exception_type diff --git a/src/test/resources/fake-agency-overlapping-trips/routes.txt b/src/test/resources/fake-agency-overlapping-trips/routes.txt new file mode 100755 index 000000000..8a0f40172 --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/routes.txt @@ -0,0 +1,2 @@ +agency_id,route_id,route_short_name,route_long_name,route_desc,route_type,route_url,route_color,route_text_color,route_branding_url +100000000,10000000,100,,,3,,7CE6E7,FFFFFF, diff --git a/src/test/resources/fake-agency-overlapping-trips/stop_times.txt b/src/test/resources/fake-agency-overlapping-trips/stop_times.txt new file mode 100755 index 000000000..2ce4c8570 --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/stop_times.txt @@ -0,0 +1,5 @@ +trip_id,arrival_time,departure_time,stop_id,stop_sequence,stop_headsign,pickup_type,drop_off_type +1A00000,07:00:00,07:00:00,A000000,1,,0,0 +1A00000,07:10:30,07:10:30,B000000,2,,0,0 +2A00000,07:00:00,07:00:00,A000000,1,,0,0 +2A00000,07:02:00,07:02:00,B000000,2,,0,0 diff --git a/src/test/resources/fake-agency-overlapping-trips/stops.txt b/src/test/resources/fake-agency-overlapping-trips/stops.txt new file mode 100755 index 000000000..0f35743a2 --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/stops.txt @@ -0,0 +1,3 @@ +stop_id,stop_code,stop_name,stop_desc,stop_lat,stop_lon,zone_id,stop_url,location_type,parent_station,stop_timezone,wheelchair_boarding +A000000,,Butler Ln,,37.0612132,-122.0074332,,,0,,, +B000000,,Scotts Valley Dr & Victor Sq,,37.0590172,-122.0096058,,,0,,, diff --git a/src/test/resources/fake-agency-overlapping-trips/trips.txt b/src/test/resources/fake-agency-overlapping-trips/trips.txt new file mode 100755 index 000000000..b1b70c9d7 --- /dev/null +++ b/src/test/resources/fake-agency-overlapping-trips/trips.txt @@ -0,0 +1,3 @@ +route_id,trip_id,direction_id,block_id,bikes_allowed,wheelchair_accessible,service_id +10000000,1A00000,0,BLOCK_1,0,0,A +10000000,2A00000,0,BLOCK_1,0,0,A From fb4565183808d9cadccc721aaa6e6c872869bf55 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Wed, 13 Mar 2019 18:05:05 -0400 Subject: [PATCH 2/6] fix(validate): fix rounded travel times check --- .../java/com/conveyal/gtfs/validator/SpeedTripValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java index 96170bbff..ef28671a7 100644 --- a/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/SpeedTripValidator.java @@ -61,7 +61,7 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< registerError(currStopTime, DEPARTURE_BEFORE_ARRIVAL); } // Detect if travel times are rounded off to minutes. - boolean bothTravelTimesRounded = areTravelTimesRounded(prevStopTime); + boolean bothTravelTimesRounded = areTravelTimesRounded(prevStopTime) && areTravelTimesRounded(currStopTime); double travelTimeSeconds = currStopTime.arrival_time - prevStopTime.departure_time; // If travel times are rounded and travel time is zero, determine the maximum and minimum possible speed // by adding/removing one minute of slack. From 1ca64050e60741e32927eb518c8ba36309a998ee Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Mon, 25 Mar 2019 10:02:15 -0400 Subject: [PATCH 3/6] refactor(overlapping-trips): add comments, clean up files --- .../conveyal/gtfs/error/NewGTFSErrorType.java | 2 +- .../error/OverlappingTripsInBlockError.java | 27 --------------- .../validator/OverlappingTripValidator.java | 33 +++++-------------- src/test/java/com/conveyal/gtfs/GTFSTest.java | 2 +- 4 files changed, 10 insertions(+), 54 deletions(-) delete mode 100644 src/main/java/com/conveyal/gtfs/error/OverlappingTripsInBlockError.java diff --git a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java index b35037642..ee89937f4 100644 --- a/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java +++ b/src/main/java/com/conveyal/gtfs/error/NewGTFSErrorType.java @@ -59,7 +59,7 @@ public enum NewGTFSErrorType { TRAVEL_TIME_ZERO(Priority.HIGH, "The vehicle arrives at this stop at the same time it departs from the previous stop."), MISSING_ARRIVAL_OR_DEPARTURE(Priority.MEDIUM, "First and last stop times are required to have both an arrival and departure time."), TRIP_TOO_FEW_STOP_TIMES(Priority.MEDIUM, "A trip must have at least two stop times to represent travel."), - TRIP_OVERLAP_IN_BLOCK(Priority.MEDIUM, "Blocks"), + TRIP_OVERLAP_IN_BLOCK(Priority.MEDIUM, "A trip overlaps another trip and shares the same block_id."), TRAVEL_TOO_SLOW(Priority.MEDIUM, "The vehicle is traveling very slowly to reach this stop from the previous one."), TRAVEL_TOO_FAST(Priority.MEDIUM, "The vehicle travels extremely fast to reach this stop from the previous one."), VALIDATOR_FAILED(Priority.HIGH, "The specified validation stage failed due to an error encountered during loading. This is likely due to an error encountered during loading (e.g., a date or number field is formatted incorrectly.)."), diff --git a/src/main/java/com/conveyal/gtfs/error/OverlappingTripsInBlockError.java b/src/main/java/com/conveyal/gtfs/error/OverlappingTripsInBlockError.java deleted file mode 100644 index 62a5a374a..000000000 --- a/src/main/java/com/conveyal/gtfs/error/OverlappingTripsInBlockError.java +++ /dev/null @@ -1,27 +0,0 @@ -package com.conveyal.gtfs.error; - -import com.conveyal.gtfs.model.Route; -import com.conveyal.gtfs.validator.model.Priority; - -import java.io.Serializable; - -/** - * Created by landon on 5/6/16. - */ -public class OverlappingTripsInBlockError extends GTFSError implements Serializable { - public static final long serialVersionUID = 1L; - - public final String[] tripIds; - public final Priority priority = Priority.HIGH; - public final String routeId; - - public OverlappingTripsInBlockError(long line, String field, String affectedEntityId, String routeId, String[] tripIds) { - super("trips", line, field, affectedEntityId); - this.tripIds = tripIds; - this.routeId = routeId; - } - - @Override public String getMessage() { - return String.format("Trip Ids %s overlap (route: %s) and share block ID %s", String.join(" & ", tripIds), routeId, affectedEntityId); - } -} diff --git a/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java index 422158aff..0c860bf6b 100644 --- a/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java @@ -6,8 +6,6 @@ import com.conveyal.gtfs.model.Stop; import com.conveyal.gtfs.model.StopTime; import com.conveyal.gtfs.model.Trip; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.time.LocalDate; import java.util.ArrayList; @@ -20,10 +18,13 @@ import static com.conveyal.gtfs.error.NewGTFSErrorType.TRIP_OVERLAP_IN_BLOCK; /** + * This validator checks that trips which run on the same block (i.e., share a block_id) do not overlap. The block_id + * represents a vehicle in service, so there must not be any trips on the same block interval that start while another + * block trip is running. + * * Created by landon on 5/2/16. */ public class OverlappingTripValidator extends TripValidator { - private static final Logger LOG = LoggerFactory.getLogger(OverlappingTripValidator.class); // check for overlapping trips within block private HashMap> blockIntervals = new HashMap<>(); @@ -34,7 +35,7 @@ public OverlappingTripValidator(Feed feed, SQLErrorStorage errorStorage) { @Override public void validateTrip(Trip trip, Route route, List stopTimes, List stops) { if (trip.block_id != null) { - // If the trip has a block_id, add a new block interval to the + // If the trip has a block_id, add a new block interval to the map. BlockInterval blockInterval = new BlockInterval(); blockInterval.trip = trip; StopTime firstStopTime = stopTimes.get(0); @@ -85,6 +86,9 @@ public void complete (ValidationResult validationResult) { } } + /** + * A simple class used during validation to store details the run interval for a block trip. + */ private class BlockInterval { Trip trip; Integer startTime; @@ -92,26 +96,5 @@ private class BlockInterval { StopTime lastStop; } - // FIXME what is this patternId? This seems like a subset of block overlap errors (within a service day). -// String patternId = feed.tripPatternMap.get(tripId); -// String patternName = feed.patterns.get(patternId).name; -// int firstDeparture = Iterables.get(stopTimes, 0).departure_time; -// int lastArrival = Iterables.getLast(stopTimes).arrival_time; -// -// String tripKey = trip.service_id + "_"+ blockId + "_" + firstDeparture +"_" + lastArrival + "_" + patternId; -// -// if (duplicateTripHash.containsKey(tripKey)) { -// String firstDepartureString = LocalTime.ofSecondOfDay(Iterables.get(stopTimes, 0).departure_time % 86399).toString(); -// String lastArrivalString = LocalTime.ofSecondOfDay(Iterables.getLast(stopTimes).arrival_time % 86399).toString(); -// String duplicateTripId = duplicateTripHash.get(tripKey); -// Trip duplicateTrip = feed.trips.get(duplicateTripId); -// long line = trip.id > duplicateTrip.id ? trip.id : duplicateTrip.id; -// feed.errors.add(new DuplicateTripError(trip, line, duplicateTripId, patternName, firstDepartureString, lastArrivalString)); -// isValid = false; -// } else { -// duplicateTripHash.put(tripKey, tripId); -// } - - } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 789bc940a..61030bc91 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -134,7 +134,7 @@ public void canLoadFeedWithOverlappingTrips () { PersistenceExpectation[] expectations = PersistenceExpectation.list( new PersistenceExpectation( new ErrorExpectation[]{ - new ErrorExpectation("error_type", "TRIP_OVERLAP_IN_BLOCK") + new ErrorExpectation("error_type", NewGTFSErrorType.TRIP_OVERLAP_IN_BLOCK.toString()) } ) ); From 01a7a193718788fc3751b9731c6043df87b2c399 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 28 Mar 2019 09:59:03 -0400 Subject: [PATCH 4/6] chore(test): fix copy-pasta javadoc --- src/test/java/com/conveyal/gtfs/GTFSTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 61030bc91..257d9c06e 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -127,7 +127,7 @@ public void canLoadFeedWithBadDates () { } /** - * Tests that a GTFS feed with bad date values in calendars.txt and calendar_dates.txt can pass the integration test. + * Tests that a GTFS feed with overlapping block trips will record the appropriate error. */ @Test public void canLoadFeedWithOverlappingTrips () { From e3fbc179b262a45cb6b94b4568fc51b408d91de1 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Thu, 28 Mar 2019 10:17:52 -0400 Subject: [PATCH 5/6] refactor(overlapping-trips): move block overlap check to ServiceValidator --- .../gtfs/validator/NewTripTimesValidator.java | 3 - .../validator/OverlappingTripValidator.java | 100 ---------------- .../gtfs/validator/ServiceValidator.java | 112 +++++++++++++++--- 3 files changed, 94 insertions(+), 121 deletions(-) delete mode 100644 src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java diff --git a/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java b/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java index 3d14e1541..3b6cf40ff 100644 --- a/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/NewTripTimesValidator.java @@ -48,9 +48,6 @@ public NewTripTimesValidator(Feed feed, SQLErrorStorage errorStorage) { new ReferencesTripValidator(feed, errorStorage), new ReversedTripValidator(feed, errorStorage), new ServiceValidator(feed, errorStorage), - // Overlapping trips validator requires use of service info computed by ServiceValidator, so it must run - // after the ServiceValidator. - new OverlappingTripValidator(feed, errorStorage), new PatternFinderValidator(feed, errorStorage) }; } diff --git a/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java b/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java deleted file mode 100644 index 0c860bf6b..000000000 --- a/src/main/java/com/conveyal/gtfs/validator/OverlappingTripValidator.java +++ /dev/null @@ -1,100 +0,0 @@ -package com.conveyal.gtfs.validator; - -import com.conveyal.gtfs.error.SQLErrorStorage; -import com.conveyal.gtfs.loader.Feed; -import com.conveyal.gtfs.model.Route; -import com.conveyal.gtfs.model.Stop; -import com.conveyal.gtfs.model.StopTime; -import com.conveyal.gtfs.model.Trip; - -import java.time.LocalDate; -import java.util.ArrayList; -import java.util.Comparator; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Set; - -import static com.conveyal.gtfs.error.NewGTFSErrorType.TRIP_OVERLAP_IN_BLOCK; - -/** - * This validator checks that trips which run on the same block (i.e., share a block_id) do not overlap. The block_id - * represents a vehicle in service, so there must not be any trips on the same block interval that start while another - * block trip is running. - * - * Created by landon on 5/2/16. - */ -public class OverlappingTripValidator extends TripValidator { - // check for overlapping trips within block - private HashMap> blockIntervals = new HashMap<>(); - - public OverlappingTripValidator(Feed feed, SQLErrorStorage errorStorage) { - super(feed, errorStorage); - } - - @Override - public void validateTrip(Trip trip, Route route, List stopTimes, List stops) { - if (trip.block_id != null) { - // If the trip has a block_id, add a new block interval to the map. - BlockInterval blockInterval = new BlockInterval(); - blockInterval.trip = trip; - StopTime firstStopTime = stopTimes.get(0); - blockInterval.startTime = firstStopTime.departure_time; - blockInterval.firstStop = firstStopTime; - blockInterval.lastStop = stopTimes.get(stopTimes.size() - 1); - // Construct new list of intervals if none exists for encountered block_id. - blockIntervals - .computeIfAbsent(trip.block_id, k -> new ArrayList<>()) - .add(blockInterval); - } - } - - @Override - public void complete (ValidationResult validationResult) { - // Iterate over each block and determine if there are any trips that overlap one another. - for (String blockId : blockIntervals.keySet()) { - List intervals = blockIntervals.get(blockId); - intervals.sort(Comparator.comparingInt(i -> i.startTime)); - // Iterate over each interval (except for the last) comparing it to every other interval (so the last interval - // is handled through the course of iteration). - // FIXME this has complexity of n^2, there has to be a better way. - for (int n = 0; n < intervals.size() - 1; n++) { - BlockInterval interval1 = intervals.get(n); - // Compare the interval at position N with all other intervals at position N+1 to the end of the list. - for (BlockInterval interval2 : intervals.subList(n + 1, intervals.size())) { - if (interval1.lastStop.departure_time <= interval2.firstStop.arrival_time || interval2.lastStop.departure_time <= interval1.firstStop.arrival_time) { - continue; - } - // If either trip's last departure occurs after the other's first arrival, they overlap. We still - // need to determine if they operate on the same day though. - if (interval1.trip.service_id.equals(interval2.trip.service_id)) { - // If the overlapping trips share a service_id, record an error. - registerError(interval1.trip, TRIP_OVERLAP_IN_BLOCK, interval2.trip.trip_id); - } else { - // Trips overlap but don't have the same service_id. - // Check to see if service days fall on the same days of the week. - ServiceValidator.ServiceInfo info1 = validationResult.serviceInfoForServiceId.get(interval1.trip.service_id); - ServiceValidator.ServiceInfo info2 = validationResult.serviceInfoForServiceId.get(interval2.trip.service_id); - Set overlappingDates = new HashSet<>(info1.datesActive); // use the copy constructor - overlappingDates.retainAll(info2.datesActive); - if (overlappingDates.size() > 0) { - registerError(interval1.trip, TRIP_OVERLAP_IN_BLOCK, interval2.trip.trip_id); - } - } - } - } - } - } - - /** - * A simple class used during validation to store details the run interval for a block trip. - */ - private class BlockInterval { - Trip trip; - Integer startTime; - StopTime firstStop; - StopTime lastStop; - } - -} - diff --git a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java index 075015980..e842e0f75 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java +++ b/src/main/java/com/conveyal/gtfs/validator/ServiceValidator.java @@ -27,13 +27,17 @@ import java.time.DayOfWeek; import java.time.LocalDate; import java.time.temporal.ChronoUnit; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import static com.conveyal.gtfs.error.NewGTFSErrorType.TRIP_OVERLAP_IN_BLOCK; + /** * This will validate that service date information is coherent, and attempt to deduce or validate the range of dates * covered by a GTFS feed. @@ -52,7 +56,7 @@ public class ServiceValidator extends TripValidator { private static final Logger LOG = LoggerFactory.getLogger(ServiceValidator.class); - + private HashMap> blockIntervals = new HashMap<>(); private Map serviceInfoForServiceId = new HashMap<>(); private Map dateInfoForDate = new HashMap<>(); @@ -63,6 +67,19 @@ public ServiceValidator(Feed feed, SQLErrorStorage errorStorage) { @Override public void validateTrip(Trip trip, Route route, List stopTimes, List stops) { + if (trip.block_id != null) { + // If the trip has a block_id, add a new block interval to the map. + BlockInterval blockInterval = new BlockInterval(); + blockInterval.trip = trip; + StopTime firstStopTime = stopTimes.get(0); + blockInterval.startTime = firstStopTime.departure_time; + blockInterval.firstStop = firstStopTime; + blockInterval.lastStop = stopTimes.get(stopTimes.size() - 1); + // Construct new list of intervals if none exists for encountered block_id. + blockIntervals + .computeIfAbsent(trip.block_id, k -> new ArrayList<>()) + .add(blockInterval); + } int firstStopDeparture = stopTimes.get(0).departure_time; int lastStopArrival = stopTimes.get(stopTimes.size() - 1).arrival_time; if (firstStopDeparture == Entity.INT_MISSING || lastStopArrival == Entity.INT_MISSING) { @@ -95,7 +112,11 @@ public void validateTrip(Trip trip, Route route, List stopTimes, List< */ @Override public void complete(ValidationResult validationResult) { - validationResult.serviceInfoForServiceId = serviceInfoForServiceId; + validateServiceInfo(validationResult); + validateBlocks(); + } + + private void validateServiceInfo(ValidationResult validationResult) { LOG.info("Merging calendars and calendar_dates..."); // First handle the calendar entries, which define repeating weekly schedules. @@ -106,14 +127,14 @@ public void complete(ValidationResult validationResult) { for (LocalDate date = calendar.start_date; date.isBefore(endDate) || date.isEqual(endDate); date = date.plusDays(1)) { DayOfWeek dayOfWeek = date.getDayOfWeek(); if ( (dayOfWeek == DayOfWeek.MONDAY && calendar.monday > 0) || - (dayOfWeek == DayOfWeek.TUESDAY && calendar.tuesday > 0) || - (dayOfWeek == DayOfWeek.WEDNESDAY && calendar.wednesday > 0) || - (dayOfWeek == DayOfWeek.THURSDAY && calendar.thursday > 0) || - (dayOfWeek == DayOfWeek.FRIDAY && calendar.friday > 0) || - (dayOfWeek == DayOfWeek.SATURDAY && calendar.saturday > 0) || - (dayOfWeek == DayOfWeek.SUNDAY && calendar.sunday > 0)) { + (dayOfWeek == DayOfWeek.TUESDAY && calendar.tuesday > 0) || + (dayOfWeek == DayOfWeek.WEDNESDAY && calendar.wednesday > 0) || + (dayOfWeek == DayOfWeek.THURSDAY && calendar.thursday > 0) || + (dayOfWeek == DayOfWeek.FRIDAY && calendar.friday > 0) || + (dayOfWeek == DayOfWeek.SATURDAY && calendar.saturday > 0) || + (dayOfWeek == DayOfWeek.SUNDAY && calendar.sunday > 0)) { // Service is active on this date. - validationResult.serviceInfoForServiceId.computeIfAbsent(calendar.service_id, ServiceInfo::new).datesActive.add(date); + serviceInfoForServiceId.computeIfAbsent(calendar.service_id, ServiceInfo::new).datesActive.add(date); } } } catch (Exception ex) { @@ -124,7 +145,7 @@ public void complete(ValidationResult validationResult) { // Next handle the calendar_dates, which specify exceptions to the repeating weekly schedules. for (CalendarDate calendarDate : feed.calendarDates) { - ServiceInfo serviceInfo = validationResult.serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); + ServiceInfo serviceInfo = serviceInfoForServiceId.computeIfAbsent(calendarDate.service_id, ServiceInfo::new); if (calendarDate.exception_type == 1) { // Service added, add to set for this date. serviceInfo.datesActive.add(calendarDate.date); @@ -149,13 +170,13 @@ select durations.service_id, duration_seconds, days_active from ( // Check for incoherent or erroneous services. - for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { if (serviceInfo.datesActive.isEmpty()) { // This service must have been referenced by trips but is never active on any day. registerError(NewGTFSError.forFeed(NewGTFSErrorType.SERVICE_NEVER_ACTIVE, serviceInfo.serviceId)); for (String tripId : serviceInfo.tripIds) { registerError( - NewGTFSError.forTable(Table.TRIPS, NewGTFSErrorType.TRIP_NEVER_ACTIVE) + NewGTFSError.forTable(Table.TRIPS, NewGTFSErrorType.TRIP_NEVER_ACTIVE) .setEntityId(tripId) .setBadValue(tripId)); } @@ -166,7 +187,7 @@ select durations.service_id, duration_seconds, days_active from ( } // Accumulate info about services into each date that they are active. - for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { for (LocalDate date : serviceInfo.datesActive) { dateInfoForDate.computeIfAbsent(date, DateInfo::new).add(serviceInfo); } @@ -220,7 +241,7 @@ select durations.service_id, duration_seconds, days_active from ( // Check for low or zero service, which seems to happen even when services are defined. // This will also catch cases where dateInfo was null and the new instance contains no service. registerError(NewGTFSError.forFeed(NewGTFSErrorType.DATE_NO_SERVICE, - DateField.GTFS_DATE_FORMATTER.format(date))); + DateField.GTFS_DATE_FORMATTER.format(date))); } } } @@ -248,7 +269,7 @@ select durations.service_id, duration_seconds, days_active from ( sql = String.format("insert into %s values (?, ?, ?, ?)", servicesTableName); PreparedStatement serviceStatement = connection.prepareStatement(sql); final BatchTracker serviceTracker = new BatchTracker("services", serviceStatement); - for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { serviceStatement.setString(1, serviceInfo.serviceId); serviceStatement.setInt(2, serviceInfo.datesActive.size()); serviceStatement.setInt(3, serviceInfo.getTotalServiceDurationSeconds()); @@ -265,7 +286,7 @@ select durations.service_id, duration_seconds, days_active from ( sql = String.format("insert into %s values (?, ?)", serviceDatesTableName); PreparedStatement serviceDateStatement = connection.prepareStatement(sql); final BatchTracker serviceDateTracker = new BatchTracker("service_dates", serviceDateStatement); - for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { for (LocalDate date : serviceInfo.datesActive) { if (date == null) continue; // TODO ERR? Can happen with bad data (unparseable dates). try { @@ -293,7 +314,7 @@ select durations.service_id, duration_seconds, days_active from ( String serviceDurationsTableName = feed.tablePrefix + "service_durations"; sql = String.format("create table %s (service_id varchar, route_type integer, " + - "duration_seconds integer, primary key (service_id, route_type))", serviceDurationsTableName); + "duration_seconds integer, primary key (service_id, route_type))", serviceDurationsTableName); LOG.info(sql); statement.execute(sql); sql = String.format("insert into %s values (?, ?, ?)", serviceDurationsTableName); @@ -302,7 +323,7 @@ select durations.service_id, duration_seconds, days_active from ( "service_durations", serviceDurationStatement ); - for (ServiceInfo serviceInfo : validationResult.serviceInfoForServiceId.values()) { + for (ServiceInfo serviceInfo : serviceInfoForServiceId.values()) { serviceInfo.durationByRouteType.forEachEntry((routeType, serviceDurationSeconds) -> { try { serviceDurationStatement.setString(1, serviceInfo.serviceId); @@ -369,4 +390,59 @@ public void add (ServiceInfo serviceInfo) { tripCount += serviceInfo.tripIds.size(); } } + + /** + * Checks that trips which run on the same block (i.e., share a block_id) do not overlap. The block_id + * represents a vehicle in service, so there must not be any trips on the same block interval that start while another + * block trip is running. + * + * NOTE: This validation check happens in the {@link ServiceValidator} because it depends on information derived + * about which service calendars operate on which feed dates ({@link #serviceInfoForServiceId}). + */ + private void validateBlocks () { + // Iterate over each block and determine if there are any trips that overlap one another. + for (String blockId : blockIntervals.keySet()) { + List intervals = blockIntervals.get(blockId); + intervals.sort(Comparator.comparingInt(i -> i.startTime)); + // Iterate over each interval (except for the last) comparing it to every other interval (so the last interval + // is handled through the course of iteration). + // FIXME this has complexity of n^2, there has to be a better way. + for (int n = 0; n < intervals.size() - 1; n++) { + BlockInterval interval1 = intervals.get(n); + // Compare the interval at position N with all other intervals at position N+1 to the end of the list. + for (BlockInterval interval2 : intervals.subList(n + 1, intervals.size())) { + if (interval1.lastStop.departure_time <= interval2.firstStop.arrival_time || interval2.lastStop.departure_time <= interval1.firstStop.arrival_time) { + continue; + } + // If either trip's last departure occurs after the other's first arrival, they overlap. We still + // need to determine if they operate on the same day though. + if (interval1.trip.service_id.equals(interval2.trip.service_id)) { + // If the overlapping trips share a service_id, record an error. + registerError(interval1.trip, TRIP_OVERLAP_IN_BLOCK, interval2.trip.trip_id); + } else { + // Trips overlap but don't have the same service_id. + // Check to see if service days fall on the same days of the week. + ServiceValidator.ServiceInfo info1 = serviceInfoForServiceId.get(interval1.trip.service_id); + ServiceValidator.ServiceInfo info2 = serviceInfoForServiceId.get(interval2.trip.service_id); + Set overlappingDates = new HashSet<>(info1.datesActive); // use the copy constructor + overlappingDates.retainAll(info2.datesActive); + if (overlappingDates.size() > 0) { + registerError(interval1.trip, TRIP_OVERLAP_IN_BLOCK, interval2.trip.trip_id); + } + } + } + } + } + } + + + /** + * A simple class used during validation to store details the run interval for a block trip. + */ + private class BlockInterval { + Trip trip; + Integer startTime; + StopTime firstStop; + StopTime lastStop; + } } From 7f41695b58680dd967b82622f412dd60c6045ff9 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Fri, 29 Mar 2019 16:54:50 -0400 Subject: [PATCH 6/6] refactor(validation-result): revert changes made for overlapping-trips --- .../java/com/conveyal/gtfs/validator/ValidationResult.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java b/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java index aa65bd38c..d79a52444 100644 --- a/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java +++ b/src/main/java/com/conveyal/gtfs/validator/ValidationResult.java @@ -5,8 +5,6 @@ import java.awt.geom.Rectangle2D; import java.io.Serializable; import java.time.LocalDate; -import java.util.HashMap; -import java.util.Map; /** * An instance of this class is returned by the validator. @@ -40,7 +38,6 @@ public class ValidationResult implements Serializable { public int[] dailyRailSeconds; public int[] dailyTotalSeconds; public int[] dailyTripCounts; - public Map serviceInfoForServiceId = new HashMap<>(); public GeographicBounds fullBounds = new GeographicBounds(); public GeographicBounds boundsWithoutOutliers = new GeographicBounds(); public long validationTime;