From 89d3114bd1f27e5e9ea818131794621252604c8a Mon Sep 17 00:00:00 2001
From: Thom Chiovoloni <tchiovoloni@mozilla.com>
Date: Fri, 19 Jan 2018 15:25:40 -0500
Subject: [PATCH] Avoid using the all_datoms table when a type requirement
 prevents strings

---
 query-algebrizer/src/clauses/mod.rs | 17 +++++++++++++----
 query-translator/tests/translate.rs | 22 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs
index e587fdeb..6d1856af 100644
--- a/query-algebrizer/src/clauses/mod.rs
+++ b/query-algebrizer/src/clauses/mod.rs
@@ -406,6 +406,14 @@ impl ConjoiningClauses {
         self.known_types.get(var).cloned().unwrap_or(ValueTypeSet::any())
     }
 
+    fn required_type_set(&self, var: &Variable) -> ValueTypeSet {
+        self.required_types.get(var).cloned().unwrap_or(ValueTypeSet::any())
+    }
+
+    fn possible_type_set(&self, var: &Variable) -> ValueTypeSet {
+        self.known_type_set(var).intersection(&self.required_type_set(var))
+    }
+
     pub fn bind_column_to_var<C: Into<Column>>(&mut self, schema: &Schema, table: TableAlias, column: C, var: Variable) {
         let column = column.into();
         // Do we have an external binding for this?
@@ -726,10 +734,11 @@ impl ConjoiningClauses {
                 // the query. If it's not, we don't need to use all_datoms here.
                 &PatternValuePlace::Variable(ref v) => {
                     // Do we know that this variable can't be a string? If so, we don't need
-                    // AllDatoms. None or String means it could be or definitely is.
-                    match self.known_types.get(v).map(|types| types.contains(ValueType::String)) {
-                        Some(false) => DatomsTable::Datoms,
-                        _           => DatomsTable::AllDatoms,
+                    // AllDatoms.
+                    if !self.possible_type_set(v).contains(ValueType::String) {
+                        DatomsTable::Datoms
+                    } else {
+                        DatomsTable::AllDatoms
                     }
                 }
                 &PatternValuePlace::Constant(NonIntegerConstant::Text(_)) =>
diff --git a/query-translator/tests/translate.rs b/query-translator/tests/translate.rs
index 5185c1df..67406686 100644
--- a/query-translator/tests/translate.rs
+++ b/query-translator/tests/translate.rs
@@ -330,6 +330,28 @@ fn test_type_required_boolean() {
     assert_eq!(args, vec![]);
 }
 
+#[test]
+fn test_type_require_avoids_all_datoms() {
+    let schema = Schema::default();
+    // Since the constraint is first, we know we don't need to use all_datoms.
+    let query = r#"[:find ?x :where [(keyword ?e)] [?x _ ?e]]"#;
+    let SQLQuery { sql, args } = translate(&schema, query);
+
+    assert_eq!(sql, "SELECT DISTINCT `datoms00`.e AS `?x` \
+                     FROM `datoms` AS `datoms00` \
+                     WHERE (`datoms00`.value_type_tag = 13)");
+    assert_eq!(args, vec![]);
+
+    // Strings always need to use all_datoms.
+    let query = r#"[:find ?x :where [(string ?e)] [?x _ ?e]]"#;
+    let SQLQuery { sql, args } = translate(&schema, query);
+
+    assert_eq!(sql, "SELECT DISTINCT `all_datoms00`.e AS `?x` \
+                     FROM `all_datoms` AS `all_datoms00` \
+                     WHERE (`all_datoms00`.value_type_tag = 10)");
+    assert_eq!(args, vec![]);
+}
+
 #[test]
 fn test_numeric_less_than_unknown_attribute() {
     let schema = Schema::default();