From bffefe7e6bcb0b5580979a8a782f0a97185f649a Mon Sep 17 00:00:00 2001
From: Richard Newman <rnewman@twinql.com>
Date: Tue, 18 Apr 2017 13:50:58 -0700
Subject: [PATCH] Review comments for #418.

---
 core/src/lib.rs                        | 14 +++++++-------
 query-algebrizer/src/clauses/inputs.rs |  9 ++++++---
 query-algebrizer/src/clauses/mod.rs    | 14 +++++++++++---
 src/errors.rs                          |  4 ++--
 4 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/core/src/lib.rs b/core/src/lib.rs
index 67b0aa63..943a30d2 100644
--- a/core/src/lib.rs
+++ b/core/src/lib.rs
@@ -61,13 +61,13 @@ impl ValueType {
 impl fmt::Display for ValueType {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(f, "{}", match *self {
-            ValueType::Ref =>     "db.type/ref",
-            ValueType::Boolean => "db.type/boolean",
-            ValueType::Instant => "db.type/instant",
-            ValueType::Long =>    "db.type/long",
-            ValueType::Double =>  "db.type/double",
-            ValueType::String =>  "db.type/string",
-            ValueType::Keyword => "db.type/keyword",
+            ValueType::Ref =>     ":db.type/ref",
+            ValueType::Boolean => ":db.type/boolean",
+            ValueType::Instant => ":db.type/instant",
+            ValueType::Long =>    ":db.type/long",
+            ValueType::Double =>  ":db.type/double",
+            ValueType::String =>  ":db.type/string",
+            ValueType::Keyword => ":db.type/keyword",
         })
     }
 }
diff --git a/query-algebrizer/src/clauses/inputs.rs b/query-algebrizer/src/clauses/inputs.rs
index 55f8daa1..4644fc4d 100644
--- a/query-algebrizer/src/clauses/inputs.rs
+++ b/query-algebrizer/src/clauses/inputs.rs
@@ -19,8 +19,7 @@ use mentat_query::{
     Variable,
 };
 
-pub use errors::{
-    Error,
+use errors::{
     ErrorKind,
     Result,
 };
@@ -32,13 +31,17 @@ pub use errors::{
 /// When built correctly, `types` is guaranteed to contain the types of `values` -- use
 /// `QueryInputs::new` or `QueryInputs::with_values` to construct an instance.
 pub struct QueryInputs {
+    // These should be crate-private.
     pub types: BTreeMap<Variable, ValueType>,
     pub values: BTreeMap<Variable, TypedValue>,
 }
 
 impl Default for QueryInputs {
     fn default() -> Self {
-        QueryInputs { types: BTreeMap::default(), values: BTreeMap::default() }
+        QueryInputs {
+            types: BTreeMap::default(),
+            values: BTreeMap::default(),
+        }
     }
 }
 
diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs
index c7d0822f..188411c5 100644
--- a/query-algebrizer/src/clauses/mod.rs
+++ b/query-algebrizer/src/clauses/mod.rs
@@ -8,6 +8,8 @@
 // CONDITIONS OF ANY KIND, either express or implied. See the License for the
 // specific language governing permissions and limitations under the License.
 
+use std::cmp;
+
 use std::collections::{
     BTreeMap,
     BTreeSet,
@@ -116,14 +118,20 @@ impl<K: Clone + Ord, V: Clone> Intersection<K> for BTreeMap<K, V> {
     /// Remove all keys from the map that are not present in `ks`.
     /// This implementation is terrible because there's no mutable iterator for BTreeMap.
     fn keep_intersected_keys(&mut self, ks: &BTreeSet<K>) {
-        let mut to_remove = Vec::with_capacity(self.len() - ks.len());
-        {
+        if self.is_empty() {
+            return;
+        }
+        if ks.is_empty() {
+            self.clear();
+        }
+
+        let expected_remaining = cmp::max(0, self.len() - ks.len());
+        let mut to_remove = Vec::with_capacity(expected_remaining);
         for k in self.keys() {
             if !ks.contains(k) {
                 to_remove.push(k.clone())
             }
         }
-        }
         for k in to_remove.into_iter() {
             self.remove(&k);
         }
diff --git a/src/errors.rs b/src/errors.rs
index c9172ae0..96587dc8 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -43,8 +43,8 @@ error_chain! {
 
     errors {
         UnboundVariables(names: BTreeSet<String>) {
-            description("unbound variables at execution time")
-            display("variables {:?} unbound at execution time", names)
+            description("unbound variables at query execution time")
+            display("variables {:?} unbound at query execution time", names)
         }
 
         InvalidArgumentName(name: String) {