Repository navigation
Remove consolidated stops while importing - #1775
aaronbrethorst wants to merge 2 commits 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
c510799 to
dc4d63b
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 dc4d63b. Looks correct; one ordering note.
- On this branch alone, a trip whose stop times are all dropped (
only_droppedin the fixture) is still stored intripswith no stop times, sincestopTimes()returns early but nothing removes the trip row. #1777 fixes that by deleting it. Worth keeping #1775 and #1777 landing together (or folding the trip removal in here) somainnever has trips without stop times. - Clearing
stop_consolidationsbeforestopsis the right order for thecanonical_idFK, and the RABA digests confirm nothing else moves.
Generated by Claude Code
A stop consolidation file removes the stops it absorbs. GtfsData now carries StopReplacements, keyed by absorbed stop ID, and the import applies them the way Java's bundle builder does: - absorbed stops are not stored - their stop times move to the canonical stop, or are dropped when the canonical stop is not stored (Java leaves them pointing at nothing, which maglev cannot store) - trip ends, and so the block indexes, see the stops after this, and a trip left with no stop times has no ends Java does not rewrite parent_station, so a child of an absorbed station points at a stop that no longer exists. Maglev's schema does not allow that, so the child points at the canonical stop, or at no parent when its stop times would be dropped. Each absorbed stop is recorded in a new stop_consolidations table in the same transaction, with its name and location. A restart that skips an unchanged import does not read the file again, so the API and realtime will need this table to resolve absorbed IDs. Nothing sets StopReplacements yet.
SonarCloud flags a DELETE with no WHERE clause as a blocker and an ORDER BY with no direction as a smell. Clearing the table is the intent, so say so with WHERE TRUE, as ClearStopAgencies already does, and sort absorbed IDs ASC.
dc4d63b to
58efad6
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. |
Part of #1977
Draft. Stacked on #1774. It stays draft until multi-feed phases B and C land.
What it does
GtfsDatagetsStopReplacements, keyed by absorbed stop ID. The import applies them the way Java's bundle builder does:StopTimeEntriesFactorydrops those too.parent_station, but maglev's schema needs a valid parent.stop_consolidationstable in the same transaction. Thestop/{id}alias and realtime translation will read it, because a restart that skips an unchanged import never reads the file again.Nothing sets
StopReplacementsyet; #1776 does.Root cause
Maglev's import has no way to merge stops. That's the gap stop consolidation fills.
Testing
mainbefore this change: n/a (new feature).TestStoreGtfsData_ConsolidatesStops,TestFeedRows_TripEndsUseConsolidatedStopsandTestStoreGtfsData_ReimportClearsStopConsolidations.stop_consolidationstable, so nothing changes until a file is configured.make testandgo vet(both tag sets) pass.Checklist
make models.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
gtfsdbtakes plain data (*gtfs.Stoppointers) and never importsinternal/consolidation. That package importsinternal/utils, which importsgtfsdb, so the reverse import would be a cycle.