clean up logic for keeping referenced elements, add tests

Author: jacobwhall

src/filter/mod.rs

@@ -101,7 +101,7 @@ fn get_referenced_ids(
 pub fn build_keep_list(
     filters: &Vec<Box<dyn ElementFilter>>,
     chunk_receiver: Receiver<Chunk<Box<[Element]>>>,
-) -> HashSet<i64> {
+) -> Vec<i64> {
     let mut keep_ids = HashSet::new();
 
     // HashMap that stores every relation ID, along with
@@ -154,14 +154,14 @@ pub fn build_keep_list(
     }
 
     // Return all IDs of kept elements + their references.
-    keep_ids_with_references
+    keep_ids_with_references.into_iter().collect()
 }
 
 /// Transform a Vec of boxed ElementFilters into a single function that filters an ElementChunk
 pub fn build_filter(
     filters: Vec<Box<dyn ElementFilter>>,
-) -> Box<dyn Fn(ElementChunk) -> ElementChunk + Sync> {
-    Box::new(move |chunk: ElementChunk| {
+) -> Box<dyn Fn(ElementChunk, Vec<i64>) -> ElementChunk + Sync> {
+    Box::new(move |chunk: ElementChunk, keep_ids: Vec<i64>| {
         let mut out_elements = Vec::new();
         for mut element in chunk.content.into_iter() {
             let mut keep_element = true;
@@ -173,6 +173,8 @@ pub fn build_filter(
             }
             if keep_element {
                 out_elements.push(element);
+            } else if keep_ids.contains(&element.id) {
+                out_elements.push(element);
             }
         }
         Chunk {

src/readers/mod.rs

@@ -6,16 +6,13 @@ use std::{
     fs,
     io::{BufRead, BufReader, Read, stdin},
     path::PathBuf,
-    sync::{
-        Arc, Mutex,
-        mpsc::{Receiver, Sender, channel},
-    },
+    sync::mpsc::{Receiver, Sender, channel},
     thread,
 };
 
 use crate::{
     SkywayError,
-    chunks::{Chunk, ChunkBuilder, ElementChunk},
+    chunks::{ChunkBuilder, ElementChunk},
     elements::Metadata,
     sort::{ElementSorter, SortStrategy},
 };
@@ -172,7 +169,7 @@ pub trait Reader: Sized + Clone + Send + 'static {
                 // and run those resulting ElementChunks through the combined_filter.
                 let chunk_iterator = self
                     .read_file(source, metadata_sender, chunk_builder)
-                    .map(|chunk| combined_filter(chunk));
+                    .map(|chunk| combined_filter(chunk, Vec::new()));
 
                 // Send out each of those filtered ElementChunks through the correct channel.
                 chunk_iterator.for_each(|chunk| {
@@ -225,10 +222,6 @@ pub trait Reader: Sized + Clone + Send + 'static {
                 // Please note that this is where all filtering happens!
                 let keep_ids = build_keep_list(&filters, first_filter_chunk_receiver);
 
-                // Make keep_ids thread-safe
-                // TODO: Consider performance implications of this.
-                let keep_ids = Arc::new(Mutex::new(keep_ids));
-
                 // "Fake" channel that we won't use, to make the reader thread happy.
                 // We already read the metadata the first time around.
                 let (fake_metadata_sender, fake_metadata_receiver) = channel();
@@ -250,37 +243,13 @@ pub trait Reader: Sized + Clone + Send + 'static {
                     // It is imperative that we re-run the filters, because even though
                     // we already know which elements we want to keep, we don't know
                     // what modifications the filters might make to those elements.
-                    .map(|chunk| combined_filter(chunk))
-                    // keep elements depending on if we determined they are necessary above
-                    .for_each(move |chunk| {
-                        // Vec that will hold each element we keep from this ElementChunk.
-                        let mut elements = Vec::new();
-
-                        // We are iterating through a ParallelIterator, which is why I am
-                        // locking keep_ids here. I reckon there are better ways of
-                        // accomplishing this!
-                        let mut keep_ids_lock = keep_ids.lock().unwrap();
-
-                        // For each element, push it to the elements Vec (to keep) if it
-                        // existed in the keep_ids Vec. At the same time, remove the ID
-                        // from keep_ids.
-                        //
-                        // TODO: is it really necessary to delete it? Are we really going
-                        // to check if keep_ids is fully exhausted in the end?
-                        for element in chunk.content {
-                            if keep_ids_lock.remove(&element.id) {
-                                elements.push(element);
-                            }
-                        }
-                        // Drop the lock so parallel processes can have their fun with keep_ids
-                        drop(keep_ids_lock);
-
-                        // Send elements Vec out, neatly packaged as a Chunk
+                    // We pass keep_ids to combined_filter so that it keeps referenced
+                    // elements.
+                    .map(|chunk| combined_filter(chunk, keep_ids.clone()))
+                    // Send chunk out for sorting
+                    .for_each(|chunk| {
                         filter_chunk_sender
-                            .send(Chunk {
-                                content: elements.into_boxed_slice(),
-                                index: chunk.index,
-                            })
+                            .send(chunk)
                             .expect("Unable to send chunk.")
                     });
 

tests/filter/mod.rs

@@ -1,2 +1,4 @@
+mod references;
+
 //mod cel;
 mod skyfilter;

tests/filter/references/mod.rs

@@ -0,0 +1,5 @@
+use crate::define_test;
+
+const CURRENT_DIR: &[&str] = &["filter", "references"];
+
+define_test!(way_references);

tests/filter/references/way_references/config.toml

@@ -0,0 +1,5 @@
+name = "Keep Way References"
+description = "Ensure that nodes are kept if ways that reference them are kept."
+input = "input.opl"
+filters = ["keep_one_way.skyfilter"]
+expected_output = "expected.opl"

tests/filter/references/way_references/expected.opl

@@ -0,0 +1,4 @@
+n3097457072 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.4512421 y37.529974200000005
+n3097457044 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.4513172 y37.529842300000006
+n3097457089 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.45114550000001 y37.529782700000005
+w305163745 v3 dV c26354328 t2014-10-27T01:57:32Z i1408522 uOmnific Tbuilding=yes,name=Abandoned%20%Environmental%20%Center Nn3097457072,n3097457089,n3097457044,n3097457072

tests/filter/references/way_references/input.opl

@@ -0,0 +1,8 @@
+n3097457039 v2 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.45103040000001 y37.5297835
+n3097457072 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.4512421 y37.529974200000005
+n3097457124 v3 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.47178100000001 y37.5249898
+n3097457044 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.4513172 y37.529842300000006
+n3104201899 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.4553514 y37.5294892
+n3097457089 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.45114550000001 y37.529782700000005
+n3104204256 v1 dV c0 t1970-01-01T00:00:00Z i0 u T x-77.45474250000001 y37.5300188
+w305163745 v3 dV c26354328 t2014-10-27T01:57:32Z i1408522 uOmnific Tbuilding=yes,name=Abandoned%20%Environmental%20%Center Nn3097457072,n3097457089,n3097457044,n3097457072

tests/filter/references/way_references/keep_one_way.skyfilter

@@ -0,0 +1,5 @@
+SkyFilter v0.6.0
+
+HAS "building"
+	COMMIT
+DROP