Use Index<Stop> instead of Arc<Stop>

POC to see if the ergonomics are ok or not

Author: antoine-de

Cargo.toml

@@ -23,6 +23,8 @@ sha2 = "0.10"
 zip = "0.5"
 thiserror = "1"
 rgb = "0.8"
+#typed-generational-arena = "0.2"
+typed-generational-arena = { git = "https://gitlab.com/antoine-de/typed-generational-arena", branch = "changes" }
 
 futures = { version = "0.3", optional = true }
 reqwest = { version = "0.11", optional = true, features = ["blocking"]}

examples/gtfs_reading.rs

@@ -12,4 +12,22 @@ fn main() {
 
     let route_1 = gtfs.routes.get("1").expect("no route 1");
     println!("{}: {:?}", route_1.short_name, route_1);
+
+    let trip = gtfs
+        .trips
+        .get("trip1")
+        .expect("impossible to find trip trip1");
+
+    let stop_time = trip
+        .stop_times
+        .iter()
+        .next()
+        .expect("no stop times in trips");
+
+    let stop = gtfs
+        .stops
+        .get(stop_time.stop)
+        .expect("no stop in stop time");
+
+    println!("first stop of trip 'trip1': {}", &stop.name);
 }

src/collection.rs

@@ -0,0 +1,75 @@
+use std::{collections::hash_map::Entry, collections::HashMap, iter::FromIterator};
+use typed_generational_arena::{Arena, Index, Iter};
+
+use crate::{Error, Id};
+
+pub struct CollectionWithID<T> {
+    storage: Arena<T>,
+    ids: HashMap<String, Index<T>>,
+}
+
+impl<T> Default for CollectionWithID<T> {
+    fn default() -> Self {
+        CollectionWithID {
+            storage: Arena::default(),
+            ids: HashMap::default(),
+        }
+    }
+}
+
+impl<T: Id> CollectionWithID<T> {
+    pub fn insert(&mut self, o: T) -> Result<Index<T>, Error> {
+        let id = o.id().to_owned();
+        match self.ids.entry(id) {
+            Entry::Occupied(_) => Err(Error::DuplicateStop(o.id().to_owned())),
+            Entry::Vacant(e) => {
+                let index = self.storage.insert(o);
+                e.insert(index);
+                Ok(index)
+            }
+        }
+    }
+}
+
+impl<T> CollectionWithID<T> {
+    pub fn get(&self, i: Index<T>) -> Option<&T> {
+        self.storage.get(i)
+    }
+
+    pub fn get_by_id(&self, id: &str) -> Option<&T> {
+        self.ids.get(id).and_then(|idx| self.storage.get(*idx))
+    }
+
+    pub fn get_mut_by_id(&mut self, id: &str) -> Option<&mut T> {
+        let idx = self.ids.get(id)?;
+        self.storage.get_mut(*idx)
+    }
+
+    pub fn get_index(&self, id: &str) -> Option<&Index<T>> {
+        self.ids.get(id)
+    }
+
+    pub fn len(&self) -> usize {
+        self.storage.len()
+    }
+
+    /// Iterates over the `(Index<T>, &T)` of the `CollectionWithID`.
+    pub fn iter(&self) -> Iter<'_, T> {
+        self.storage.iter()
+    }
+}
+
+impl<T: Id> FromIterator<T> for CollectionWithID<T> {
+    fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
+        let mut c = Self::default();
+
+        for i in iter {
+            // Note FromIterator does not handle the insertion error
+            let _ = c
+                .insert(i)
+                .map_err(|e| println!("impossible to insert elt: {}", e));
+        }
+
+        c
+    }
+}

src/error.rs

@@ -28,6 +28,9 @@ pub enum Error {
     /// The color is not given in the RRGGBB format, without a leading `#`
     #[error("'{0}' is not a valid color; RRGGBB format is expected, without a leading `#`")]
     InvalidColor(String),
+    /// Several stops have the same id
+    #[error("duplicate stop: '{0}'")]
+    DuplicateStop(String),
     /// Generic Input/Output error while reading a file
     #[error("impossible to read file")]
     IO(#[from] std::io::Error),

src/gtfs.rs

@@ -1,9 +1,8 @@
-use crate::{objects::*, Error, RawGtfs};
+use crate::{collection::CollectionWithID, objects::*, Error, RawGtfs};
 use chrono::prelude::NaiveDate;
 use chrono::Duration;
 use std::collections::{HashMap, HashSet};
 use std::convert::TryFrom;
-use std::sync::Arc;
 
 /// Data structure with all the GTFS objects
 ///
@@ -27,8 +26,8 @@ pub struct Gtfs {
     pub calendar: HashMap<String, Calendar>,
     /// All calendar dates grouped by service_id
     pub calendar_dates: HashMap<String, Vec<CalendarDate>>,
-    /// All stop by `stop_id`. Stops are in an [Arc] because they are also referenced by each [StopTime]
-    pub stops: HashMap<String, Arc<Stop>>,
+    /// All stop by `stop_id`
+    pub stops: CollectionWithID<Stop>,
     /// All routes by `route_id`
     pub routes: HashMap<String, Route>,
     /// All trips by `trip_id`
@@ -49,7 +48,7 @@ impl TryFrom<RawGtfs> for Gtfs {
     ///
     /// It might fail if some mandatory files couldn’t be read or if there are references to other objects that are invalid.
     fn try_from(raw: RawGtfs) -> Result<Gtfs, Error> {
-        let stops = to_stop_map(
+        let stops = to_stop_collection(
             raw.stops?,
             raw.transfers.unwrap_or_else(|| Ok(Vec::new()))?,
             raw.pathways.unwrap_or(Ok(Vec::new()))?,
@@ -176,10 +175,9 @@ impl Gtfs {
 
     /// Gets a [Stop] by its `stop_id`
     pub fn get_stop<'a>(&'a self, id: &str) -> Result<&'a Stop, Error> {
-        match self.stops.get(id) {
-            Some(stop) => Ok(stop),
-            None => Err(Error::ReferenceError(id.to_owned())),
-        }
+        self.stops
+            .get_by_id(id)
+            .ok_or_else(|| Error::ReferenceError(id.to_owned()))
     }
 
     /// Gets a [Trip] by its `trip_id`
@@ -232,37 +230,34 @@ fn to_map<O: Id>(elements: impl IntoIterator<Item = O>) -> HashMap<String, O> {
         .collect()
 }
 
-fn to_stop_map(
+fn to_stop_collection(
     stops: Vec<Stop>,
     raw_transfers: Vec<RawTransfer>,
     raw_pathways: Vec<RawPathway>,
-) -> Result<HashMap<String, Arc<Stop>>, Error> {
-    let mut stop_map: HashMap<String, Stop> =
-        stops.into_iter().map(|s| (s.id.clone(), s)).collect();
+) -> Result<CollectionWithID<Stop>, Error> {
+    let mut stops: CollectionWithID<Stop> = stops.into_iter().collect();
 
     for transfer in raw_transfers {
-        stop_map
-            .get(&transfer.to_stop_id)
+        stops
+            .get_by_id(&transfer.to_stop_id)
             .ok_or_else(|| Error::ReferenceError(transfer.to_stop_id.to_string()))?;
-        stop_map
-            .entry(transfer.from_stop_id.clone())
-            .and_modify(|stop| stop.transfers.push(StopTransfer::from(transfer)));
+        let s = stops
+            .get_mut_by_id(&transfer.from_stop_id)
+            .ok_or_else(|| Error::ReferenceError(transfer.from_stop_id.to_string()))?;
+        s.transfers.push(StopTransfer::from(transfer));
     }
 
     for pathway in raw_pathways {
-        stop_map
-            .get(&pathway.to_stop_id)
+        stops
+            .get_by_id(&pathway.to_stop_id)
             .ok_or_else(|| Error::ReferenceError(pathway.to_stop_id.to_string()))?;
-        stop_map
-            .entry(pathway.from_stop_id.clone())
-            .and_modify(|stop| stop.pathways.push(Pathway::from(pathway)));
+        let s = stops
+            .get_mut_by_id(&pathway.from_stop_id)
+            .ok_or_else(|| Error::ReferenceError(pathway.to_stop_id.to_string()))?;
+        s.pathways.push(Pathway::from(pathway));
     }
 
-    let res = stop_map
-        .into_iter()
-        .map(|(i, s)| (i, Arc::new(s)))
-        .collect();
-    Ok(res)
+    Ok(stops)
 }
 
 fn to_shape_map(shapes: Vec<Shape>) -> HashMap<String, Vec<Shape>> {
@@ -292,7 +287,7 @@ fn create_trips(
     raw_trips: Vec<RawTrip>,
     raw_stop_times: Vec<RawStopTime>,
     raw_frequencies: Vec<RawFrequency>,
-    stops: &HashMap<String, Arc<Stop>>,
+    stops: &CollectionWithID<Stop>,
 ) -> Result<HashMap<String, Trip>, Error> {
     let mut trips = to_map(raw_trips.into_iter().map(|rt| Trip {
         id: rt.id,
@@ -313,9 +308,9 @@ fn create_trips(
             .get_mut(&s.trip_id)
             .ok_or_else(|| Error::ReferenceError(s.trip_id.to_string()))?;
         let stop = stops
-            .get(&s.stop_id)
-            .ok_or_else(|| Error::ReferenceError(s.stop_id.to_string()))?;
-        trip.stop_times.push(StopTime::from(&s, Arc::clone(stop)));
+            .get_index(&s.stop_id)
+            .ok_or_else(||Error::ReferenceError(s.stop_id.to_string()))?;
+        trip.stop_times.push(StopTime::from(&s, *stop));
     }
 
     for trip in &mut trips.values_mut() {

src/lib.rs

@@ -43,6 +43,7 @@ extern crate derivative;
 #[macro_use]
 extern crate serde_derive;
 
+mod collection;
 mod enums;
 pub mod error;
 mod gtfs;

src/objects.rs

@@ -4,7 +4,7 @@ use chrono::{Datelike, NaiveDate, Weekday};
 use rgb::RGB8;
 
 use std::fmt;
-use std::sync::Arc;
+use typed_generational_arena::Index;
 
 /// Objects that have an identifier implement this trait
 ///
@@ -14,24 +14,12 @@ pub trait Id {
     fn id(&self) -> &str;
 }
 
-impl<T: Id> Id for Arc<T> {
-    fn id(&self) -> &str {
-        self.as_ref().id()
-    }
-}
-
 /// Trait to introspect what is the object’s type (stop, route…)
 pub trait Type {
     /// What is the type of the object
     fn object_type(&self) -> ObjectType;
 }
 
-impl<T: Type> Type for Arc<T> {
-    fn object_type(&self) -> ObjectType {
-        self.as_ref().object_type()
-    }
-}
-
 /// A calender describes on which days the vehicle runs. See <https://gtfs.org/reference/static/#calendartxt>
 #[derive(Debug, Deserialize, Serialize)]
 pub struct Calendar {
@@ -258,14 +246,14 @@ pub struct RawStopTime {
 }
 
 /// The moment where a vehicle, running on [Trip] stops at a [Stop]. See <https://gtfs.org/reference/static/#stopstxt>
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug)]
 pub struct StopTime {
     /// Arrival time of the stop time.
     /// It's an option since the intermediate stops can have have no arrival
     /// and this arrival needs to be interpolated
     pub arrival_time: Option<u32>,
-    /// [Stop] where the vehicle stops
-    pub stop: Arc<Stop>,
+    /// [Index] of the [Stop] where the vehicle stops
+    pub stop: Index<Stop>,
     /// Departure time of the stop time.
     /// It's an option since the intermediate stops can have have no departure
     /// and this departure needs to be interpolated
@@ -290,7 +278,7 @@ pub struct StopTime {
 
 impl StopTime {
     /// Creates [StopTime] by linking a [RawStopTime::stop_id] to the actual [Stop]
-    pub fn from(stop_time_gtfs: &RawStopTime, stop: Arc<Stop>) -> Self {
+    pub fn from(stop_time_gtfs: &RawStopTime, stop: Index<Stop>) -> Self {
         Self {
             arrival_time: stop_time_gtfs.arrival_time,
             departure_time: stop_time_gtfs.departure_time,

src/tests.rs

@@ -357,7 +357,7 @@ fn read_interpolated_stops() {
     assert_eq!(1, gtfs.feed_info.len());
     // the second stop have no departure/arrival, it should not cause any problems
     assert_eq!(
-        gtfs.trips["trip1"].stop_times[1].stop.name,
+        gtfs.stops.get(gtfs.trips["trip1"].stop_times[1].stop).expect("no stop").name,
         "Stop Point child of 1"
     );
     assert!(gtfs.trips["trip1"].stop_times[1].arrival_time.is_none());