Repository navigation
Remove trips consolidation leaves too short - #1777
aaronbrethorst wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
The description follows the PR template. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
github-actions
Bot
added
the
follows-template
07fe079 to
afc08dd
Compare
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
burma-shave
left a comment
burma-shave
left a comment
There was a problem hiding this comment.
Reviewed at afc08dd. The removal itself is correct: trips are inserted before stop times, the removed ones never get stop-time rows, and frequencies are filtered before insert, so the only other trips FK holders don't see them. Java's TripEntriesFactory threshold (size() < 2, "missing stops!") matches minStopTimesPerTrip. One stack-ordering suggestion inline.
Generated by Claude Code
| // isTooShortAfterConsolidation reports whether stop consolidation dropped | ||
| // enough of a trip's stop times that Java would drop the trip. A trip that | ||
| // was already short in the feed is left alone, as maglev always has. | ||
| func isTooShortAfterConsolidation(original, consolidated []gtfs.ScheduledStopTime) bool { |
There was a problem hiding this comment.
#1808 deletes this function two PRs later and replaces it with a plain len(consolidated) < minStopTimesPerTrip, after moving the general "fewer than two stop times" rule into validation. Since that's the end state, it'd be less churn to land #1808's general rule first (it doesn't depend on consolidation, so it could target main on its own) and have this PR use the plain check from the start. That also lets the one-stop-trip behavior change for every feed be reviewed on its own rather than inside a draft consolidation stack.
Generated by Claude Code
|
Keeping this in draft until #1808 lands on main as a standalone PR. Once it does, I'll rebase this onto it and swap |
When stop consolidation drops the stop times of an absorbed stop whose canonical stop isn't stored, a trip can be left with one stop time or none. Java's bundle builder drops such a trip: TripEntriesFactory skips any trip with fewer than two stop times after StopTimeEntriesFactory has discarded those with no stop. The import now does the same for trips consolidation shortened. Those trips are deleted after their stop times stream past, so they get no stop times, frequencies or block entries. Maglev keeps one-stop trips that come straight from the feed, as before; Java drops those too, but that is a separate parity question.
afc08dd to
2580451
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
Part of #1977
Draft. Stacked on #1773. Draft until multi-feed phases B and C land.
What it does
When stop consolidation drops a trip's stop times so that it's left with fewer than two, the import removes the trip, as Java does. Removed trips get no stop times, frequencies or block entries.
Root cause
Java's bundle builder drops such trips in two steps:
StopTimeEntriesFactory.processStopTimesdiscards stop times with no stop. These are the references to an absorbed stop whose canonical stop isn't stored.TripEntriesFactory.processTripthen returns no trip entry for any trip left with fewer than two stop times ("trip … missing stops!").Without this change, maglev kept such trips with one stop time, or none. The import guarantees every stored trip has stop times, so a trip with none breaks that.
Testing
mainbefore this change: n/a. On Add a stop-consolidation-url config option #1773,TestStoreGtfsData_ConsolidatesStopsfails without this commit, because tripsshortandonly_droppedsurvive.make test, andgo vetwith both tag sets, pass.Checklist
mainand passes on this branch.gtfsdb/(db.go,models.go,query.sql.go) ortestdata/openapi.yml(synced from upstream).r.Context()to DB/service calls, notcontext.Background().testdata/openapi.yml, has an issue open on OneBusAway/sdk-config, or is outside the SDK and the description says why.Notes for reviewers
Java drops every trip with fewer than two stop times, not just the ones consolidation shortened. Maglev keeps one-stop trips that come straight from the feed. This PR leaves that alone; matching Java there would change a non-consolidation behavior and deserves its own issue.