Closes #634 - Fix variables in predicates (#635) r=rnewman

We were forgetting to check for bound variables when resolving types other than ref types during inequality handling. This patch adds in the binding checks and `bails` if the bound variable is of the wrong type. #634

core/src/types.rs

@@ -243,6 +243,13 @@ impl ValueType {
             ValueType::Uuid => values::DB_TYPE_UUID.clone(),
         }
     }
+
+    pub fn is_numeric(&self) -> bool {
+        match self {
+            &ValueType::Long | &ValueType::Double => true,
+            _ => false
+        }
+    }
 }
 
 impl fmt::Display for ValueType {

query-algebrizer/src/clauses/resolve.rs

@@ -45,11 +45,20 @@ impl ConjoiningClauses {
         use self::FnArg::*;
         match arg {
             FnArg::Variable(var) => {
-                self.constrain_var_to_numeric(var.clone());
+                // Handle incorrect types
-                self.column_bindings
+                if let Some(v) = self.bound_value(&var) {
-                    .get(&var)
+                    if v.value_type().is_numeric() {
-                    .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone())))
+                        Ok(QueryValue::TypedValue(v))
-                    .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))
+                    } else {
+                        bail!(ErrorKind::InputTypeDisagreement(var.name().clone(), ValueType::Long, v.value_type()));
+                    }
+                } else {
+                    self.constrain_var_to_numeric(var.clone());
+                    self.column_bindings
+                        .get(&var)
+                        .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone())))
+                        .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))
+                }
             },
             // Can't be an entid.
             EntidOrInteger(i) => Ok(QueryValue::TypedValue(TypedValue::Long(i))),
@@ -73,11 +82,17 @@ impl ConjoiningClauses {
         use self::FnArg::*;
         match arg {
             FnArg::Variable(var) => {
-                self.constrain_var_to_type(var.clone(), ValueType::Instant);
+                match self.bound_value(&var) {
-                self.column_bindings
+                    Some(TypedValue::Instant(v)) => Ok(QueryValue::TypedValue(TypedValue::Instant(v))),
-                    .get(&var)
+                    Some(v) => bail!(ErrorKind::InputTypeDisagreement(var.name().clone(), ValueType::Instant, v.value_type())),
-                    .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone())))
+                    None => {
-                    .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))
+                        self.constrain_var_to_type(var.clone(), ValueType::Instant);
+                        self.column_bindings
+                            .get(&var)
+                            .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone())))
+                            .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))
+                    },
+                }
             },
             Constant(NonIntegerConstant::Instant(v)) => {
                 Ok(QueryValue::TypedValue(TypedValue::Instant(v)))
@@ -152,10 +167,15 @@ impl ConjoiningClauses {
         use self::FnArg::*;
         match arg {
             FnArg::Variable(var) => {
-                self.column_bindings
+                match self.bound_value(&var) {
-                    .get(&var)
+                    Some(v) => Ok(QueryValue::TypedValue(v)),
-                    .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone())))
+                    None => {
-                    .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))
+                        self.column_bindings
+                            .get(&var)
+                            .and_then(|cols| cols.first().map(|col| QueryValue::Column(col.clone())))
+                            .ok_or_else(|| Error::from_kind(ErrorKind::UnboundVariable(var.name())))
+                    },
+                }
             },
             EntidOrInteger(i) => Ok(QueryValue::PrimitiveLong(i)),
             IdentOrKeyword(_) => unimplemented!(),     // TODO

query-algebrizer/tests/predicate.rs

@@ -17,7 +17,10 @@ mod utils;
 
 use mentat_core::{
     Attribute,
+    DateTime,
     Schema,
+    TypedValue,
+    Utc,
     ValueType,
     ValueTypeSet,
 };
@@ -32,11 +35,13 @@ use mentat_query_algebrizer::{
     EmptyBecause,
     ErrorKind,
     Known,
+    QueryInputs,
 };
 
 use utils::{
     add_attribute,
     alg,
+    alg_with_inputs,
     associate_ident,
     bails,
 };
@@ -45,6 +50,7 @@ fn prepopulated_schema() -> Schema {
     let mut schema = Schema::default();
     associate_ident(&mut schema, NamespacedKeyword::new("foo", "date"), 65);
     associate_ident(&mut schema, NamespacedKeyword::new("foo", "double"), 66);
+    associate_ident(&mut schema, NamespacedKeyword::new("foo", "long"), 67);
     add_attribute(&mut schema, 65, Attribute {
         value_type: ValueType::Instant,
         multival: false,
@@ -55,6 +61,11 @@ fn prepopulated_schema() -> Schema {
         multival: false,
         ..Default::default()
     });
+    add_attribute(&mut schema, 67, Attribute {
+        value_type: ValueType::Long,
+        multival: false,
+        ..Default::default()
+    });
     schema
 }
 
@@ -116,3 +127,60 @@ fn test_instant_predicates_require_instants() {
     assert_eq!(cc.known_type(&Variable::from_valid_name("?t")).expect("?t is known"),
                ValueType::Double);
 }
+
+#[test]
+fn test_instant_predicates_accepts_var() {
+    let schema = prepopulated_schema();
+    let known = Known::for_schema(&schema);
+
+    let instant_var = Variable::from_valid_name("?time");
+    let instant_value = TypedValue::Instant(DateTime::parse_from_rfc3339("2018-04-11T19:17:00.000Z")
+                    .map(|t| t.with_timezone(&Utc))
+                    .expect("expected valid date"));
+
+    let query = r#"[:find ?e
+                    :in ?time
+                    :where
+                    [?e :foo/date ?t]
+                    [(< ?t ?time)]]"#;
+    let cc = alg_with_inputs(known, query, QueryInputs::with_value_sequence(vec![(instant_var.clone(), instant_value.clone())]));
+    assert_eq!(cc.known_type(&instant_var).expect("?time is known"),
+               ValueType::Instant);
+
+    let query = r#"[:find ?e
+                    :in ?time
+                    :where
+                    [?e :foo/date ?t]
+                    [(> ?time, ?t)]]"#;
+    let cc = alg_with_inputs(known, query, QueryInputs::with_value_sequence(vec![(instant_var.clone(), instant_value.clone())]));
+    assert_eq!(cc.known_type(&instant_var).expect("?time is known"),
+               ValueType::Instant);
+}
+
+#[test]
+fn test_numeric_predicates_accepts_var() {
+    let schema = prepopulated_schema();
+    let known = Known::for_schema(&schema);
+
+    let numeric_var = Variable::from_valid_name("?long");
+    let numeric_value = TypedValue::Long(1234567);
+
+    // You can't use a string for an inequality: this is a straight-up error.
+    let query = r#"[:find ?e
+                    :in ?long
+                    :where
+                    [?e :foo/long ?t]
+                    [(> ?t ?long)]]"#;
+    let cc = alg_with_inputs(known, query, QueryInputs::with_value_sequence(vec![(numeric_var.clone(), numeric_value.clone())]));
+    assert_eq!(cc.known_type(&numeric_var).expect("?long is known"),
+               ValueType::Long);
+
+    let query = r#"[:find ?e
+                    :in ?long
+                    :where
+                    [?e :foo/long ?t]
+                    [(> ?long, ?t)]]"#;
+    let cc = alg_with_inputs(known, query, QueryInputs::with_value_sequence(vec![(numeric_var.clone(), numeric_value.clone())]));
+    assert_eq!(cc.known_type(&numeric_var).expect("?long is known"),
+               ValueType::Long);
+}

query-algebrizer/tests/utils/mod.rs

@@ -98,3 +98,8 @@ pub fn alg(known: Known, input: &str) -> ConjoiningClauses {
     let parsed = parse_find_string(input).expect("query input to have parsed");
     algebrize(known, parsed).expect("algebrizing to have succeeded").cc
 }
+
+pub fn alg_with_inputs(known: Known, input: &str, inputs: QueryInputs) -> ConjoiningClauses {
+    let parsed = parse_find_string(input).expect("query input to have parsed");
+    algebrize_with_inputs(known, parsed, 0, inputs).expect("algebrizing to have succeeded").cc
+}