Tests and fixes for aggregates over different or unknown types. (#588) r=emily

query-algebrizer/src/clauses/mod.rs

@@ -940,27 +940,39 @@ impl ConjoiningClauses {
         }
     }
 
+    /// When we're done with all patterns, we might have a set of type requirements that will
+    /// be used to add additional constraints to the execution plan.
+    ///
+    /// This function does so.
+    ///
+    /// Furthermore, those type requirements will not yet be present in `known_types`, which
+    /// means they won't be used by the projector or translator.
+    ///
+    /// This step also updates `known_types` to match.
     pub(crate) fn process_required_types(&mut self) -> Result<()> {
         if self.empty_because.is_some() {
             return Ok(())
         }
+
         // We can't call `mark_known_empty` inside the loop since it would be a
-        // mutable borrow on self while we're iterating over `self.required_types`.
+        // mutable borrow on self while we're using fields on `self`.
-        // Doing it like this avoids needing to copy `self.required_types`.
+        // We still need to clone `required_types` 'cos we're mutating in
+        // `narrow_types_for_var`.
         let mut empty_because: Option<EmptyBecause> = None;
-        for (var, types) in self.required_types.iter() {
+        for (var, types) in self.required_types.clone().into_iter() {
-            if let Some(already_known) = self.known_types.get(var) {
+            if let Some(already_known) = self.known_types.get(&var) {
-                if already_known.is_disjoint(types) {
+                if already_known.is_disjoint(&types) {
                     // If we know the constraint can't be one of the types
                     // the variable could take, then we know we're empty.
                     empty_because = Some(EmptyBecause::TypeMismatch {
-                        var: var.clone(),
+                        var: var,
                         existing: *already_known,
-                        desired: *types,
+                        desired: types,
                     });
                     break;
                 }
-                if already_known.is_subset(types) {
+
+                if already_known.is_subset(&types) {
                     // TODO: I'm not convinced that we can do nothing here.
                     //
                     // Consider `[:find ?x ?v :where [_ _ ?v] [(> ?v 10)] [?x :foo/long ?v]]`.
@@ -985,18 +997,23 @@ impl ConjoiningClauses {
                 }
             }
 
+            // Update known types.
+            self.narrow_types_for_var(var.clone(), types);
+
             let qa = self.extracted_types
                          .get(&var)
                          .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))?;
             self.wheres.add_intersection(ColumnConstraint::HasTypes {
                 value: qa.0.clone(),
-                value_types: *types,
+                value_types: types,
                 check_value: true,
             });
         }
+
         if let Some(reason) = empty_because {
             self.mark_known_empty(reason);
         }
+
         Ok(())
     }
 

tests/query.rs

@@ -33,6 +33,11 @@ use mentat_core::{
     Utc,
     Uuid,
     ValueType,
+    ValueTypeSet,
+};
+
+use mentat_query_projector::{
+    SimpleAggregationOp,
 };
 
 use mentat::{
@@ -529,6 +534,133 @@ fn test_lookup() {
     let fetched_many = conn.lookup_value_for_attribute(&c, *entid, &foo_many).unwrap().unwrap();
     assert!(two_longs.contains(&fetched_many));
 }
+
+#[test]
+fn test_aggregates_type_handling() {
+    let mut store = Store::open("").expect("opened");
+    store.transact(r#"[
+        {:db/ident :test/boolean :db/valueType :db.type/boolean :db/cardinality :db.cardinality/one}
+        {:db/ident :test/long    :db/valueType :db.type/long    :db/cardinality :db.cardinality/one}
+        {:db/ident :test/double  :db/valueType :db.type/double  :db/cardinality :db.cardinality/one}
+        {:db/ident :test/string  :db/valueType :db.type/string  :db/cardinality :db.cardinality/one}
+        {:db/ident :test/keyword :db/valueType :db.type/keyword :db/cardinality :db.cardinality/one}
+        {:db/ident :test/uuid    :db/valueType :db.type/uuid    :db/cardinality :db.cardinality/one}
+        {:db/ident :test/instant :db/valueType :db.type/instant :db/cardinality :db.cardinality/one}
+        {:db/ident :test/ref     :db/valueType :db.type/ref     :db/cardinality :db.cardinality/one}
+    ]"#).unwrap();
+
+    store.transact(r#"[
+        {:test/boolean false
+         :test/long    10
+         :test/double  2.4
+         :test/string  "one"
+         :test/keyword :foo/bar
+         :test/uuid    #uuid "55555234-1234-1234-1234-123412341234"
+         :test/instant #inst "2017-01-01T11:00:00.000Z"
+         :test/ref     1}
+        {:test/boolean true
+         :test/long    20
+         :test/double  4.4
+         :test/string  "two"
+         :test/keyword :foo/baz
+         :test/uuid    #uuid "66666234-1234-1234-1234-123412341234"
+         :test/instant #inst "2018-01-01T11:00:00.000Z"
+         :test/ref     2}
+        {:test/boolean true
+         :test/long    30
+         :test/double  6.4
+         :test/string  "three"
+         :test/keyword :foo/noo
+         :test/uuid    #uuid "77777234-1234-1234-1234-123412341234"
+         :test/instant #inst "2019-01-01T11:00:00.000Z"
+         :test/ref     3}
+    ]"#).unwrap();
+
+    // No type limits => can't do it.
+    let r = store.q_once(r#"[:find (sum ?v) . :where [_ _ ?v]]"#, None);
+    let all_types = ValueTypeSet::any();
+    match r {
+        Result::Err(
+            Error(
+                ErrorKind::TranslatorError(
+                    ::mentat_query_translator::ErrorKind::ProjectorError(
+                        ::mentat_query_projector::ErrorKind::CannotApplyAggregateOperationToTypes(
+                            SimpleAggregationOp::Sum,
+                            types
+                        ),
+                    )
+                ),
+                _)) => {
+                    assert_eq!(types, all_types);
+                },
+        r => panic!("Unexpected: {:?}", r),
+    }
+
+    // You can't sum instants.
+    let r = store.q_once(r#"[:find (sum ?v) .
+                             :where [_ _ ?v] [(instant ?v)]]"#,
+                         None);
+    match r {
+        Result::Err(
+            Error(
+                ErrorKind::TranslatorError(
+                    ::mentat_query_translator::ErrorKind::ProjectorError(
+                        ::mentat_query_projector::ErrorKind::CannotApplyAggregateOperationToTypes(
+                            SimpleAggregationOp::Sum,
+                            types
+                        ),
+                    )
+                ),
+                _)) => {
+                    assert_eq!(types, ValueTypeSet::of_one(ValueType::Instant));
+                },
+        r => panic!("Unexpected: {:?}", r),
+    }
+
+    // But you can count them.
+    let r = store.q_once(r#"[:find (count ?v) .
+                             :where [_ _ ?v] [(instant ?v)]]"#,
+                         None)
+                 .into_scalar_result()
+                 .expect("results")
+                 .unwrap();
+
+    // Our two transactions, the bootstrap transaction, plus the three values.
+    assert_eq!(TypedValue::Long(6), r);
+
+    // And you can min them, which returns an instant.
+    let r = store.q_once(r#"[:find (min ?v) .
+                             :where [_ _ ?v] [(instant ?v)]]"#,
+                         None)
+                 .into_scalar_result()
+                 .expect("results")
+                 .unwrap();
+
+    let earliest = DateTime::parse_from_rfc3339("2017-01-01T11:00:00.000Z").unwrap().with_timezone(&Utc);
+    assert_eq!(TypedValue::Instant(earliest), r);
+
+    let r = store.q_once(r#"[:find (sum ?v) .
+                             :where [_ _ ?v] [(long ?v)]]"#,
+                         None)
+                 .into_scalar_result()
+                 .expect("results")
+                 .unwrap();
+
+    // Yes, the current version is in the store as a Long!
+    let total = 30i64 + 20i64 + 10i64 + ::mentat_db::db::CURRENT_VERSION as i64;
+    assert_eq!(TypedValue::Long(total), r);
+
+    let r = store.q_once(r#"[:find (avg ?v) .
+                             :where [_ _ ?v] [(double ?v)]]"#,
+                         None)
+                 .into_scalar_result()
+                 .expect("results")
+                 .unwrap();
+
+    let avg = (6.4f64 / 3f64) + (4.4f64 / 3f64) + (2.4f64 / 3f64);
+    assert_eq!(TypedValue::Double(avg.into()), r);
+}
+
 #[test]
 fn test_type_reqs() {
     let mut c = new_connection("").expect("Couldn't open conn.");