refactor: create RoutesForLocation handler#848
Conversation
| return items, nil | ||
| } | ||
|
|
||
| const getActiveRoutesWithinBounds = ` |
There was a problem hiding this comment.
some open questions about this still:
- I need to check the query plan to make sure it's not terrible
- does the ORDER BY do the right thing in combination with the DISTINCT? For example if I have two stops for a route, will it order by the closest stop?
There was a problem hiding this comment.
my suspicion was correct, this was picking a random stop to use for the distance ordering. I've fixed this in the next commit.
query plan looks reasonable:
--SCAN r VIRTUAL TABLE INDEX 2:D0B1D2B3
|--SEARCH stops USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH stop_times USING COVERING INDEX idx_stop_times_stop_id_trip_id (stop_id=?)
|--SEARCH trips USING INDEX sqlite_autoindex_trips_1 (id=?)
|--SEARCH routes USING INDEX sqlite_autoindex_routes_1 (id=?)
|--USE TEMP B-TREE FOR GROUP BY
`--USE TEMP B-TREE FOR ORDER BY
There was a problem hiding this comment.
hey @fletcherw
Great!, I'm still testing it, and I'll add my review soon.
| ` | ||
|
|
||
| type GetActiveRoutesWithinBoundsParams struct { | ||
| MinLat float64 |
There was a problem hiding this comment.
could simplify the param count by calculating Lat/Lon as the median of Min/Max.
gtfsdb/stops_rtree.go
Outdated
| cos(radians(stops.lon) - radians(?7)) + | ||
| sin(radians(?6)) * | ||
| sin(radians(stops.lat)) | ||
| ) ASC |
There was a problem hiding this comment.
since I'm calculating the distance here, I could also pass through radius to filter out routes that are in the lat/lon bounding box but outside the radius.
| api.clientCanceledResponse(w, r, ctx.Err()) | ||
| var agencies []models.AgencyReference | ||
| for agencyID := range agencyIDs { | ||
| agency, err := api.GtfsManager.FindAgency(ctx, agencyID) |
There was a problem hiding this comment.
this could be improved with a batch agency query.
gtfsdb/stops_rtree.go
Outdated
| WHERE | ||
| r.min_lat >= ?3 AND r.max_lat <= ?4 | ||
| AND r.min_lon >= ?5 AND r.max_lon <= ?6 | ||
| AND (?7 == "" OR routes.short_name == ?7) |
There was a problem hiding this comment.
One small thing the handler lowercases the query query = strings.ToLower(sanitizedQuery) before passing it to the SQL, but the SQL compares it against routes.short_name as is, so it'll never match routes with mixed-case short names.
Fix would be LOWER(routes.short_name) == ?7.
There was a problem hiding this comment.
This was a good catch, I didn't think about this. Fortunately it turns out sqlite is case-insensitive by default if you use LIKE.
sqlite> select * from routes where short_name like 'ShuTTle' limit 10;
1-SHUTTLE|40|Shuttle|Link Replacement Shuttle Bus||3||FFB819|000000|1|1
2-SHUTTLE|40|Shuttle|2 Line Shuttle Bus||3||FFB819|000000|1|1
TLINE_S|40|Shuttle|T Line Replacement Shuttle Bus||3||DA7C00|FFFFFF|1|1
Added a test for this case as well to make sure this doesn't break in the future.
Create a new RoutesForLocation database query that encapsulates logic for routes-for-location handler. Splitting out this logic allows simplifying the StopsForLocation function and moving Go logic from the RPC handlers into SQL. Add a SQLite build flag to support math functions used for calculating haversine distance for latlng.
2 participants
Create a new RoutesForLocation database query that encapsulates logic for routes-for-location handler. Splitting out this logic allows simplifying the StopsForLocation function and moving Go logic from the RPC handlers into SQL.
Add a SQLite build flag to support math functions used for calculating haversine distance for latlng.