Address review feedback unrelated to the translator

query-algebrizer/src/clauses/inputs.rs

@@ -30,7 +30,6 @@ use errors::{
 /// the bindings that will be used at execution time.
 /// When built correctly, `types` is guaranteed to contain the types of `values` -- use
 /// `QueryInputs::new` or `QueryInputs::with_values` to construct an instance.
-#[derive(Clone)]
 pub struct QueryInputs {
     // These should be crate-private.
     pub types: BTreeMap<Variable, ValueType>,

query-algebrizer/src/clauses/mod.rs

@@ -83,8 +83,6 @@ use validate::{
     validate_or_join,
 };
 
-use self::predicate::parse_type_predicate;
-
 pub use self::inputs::QueryInputs;
 
 // We do this a lot for errors.
@@ -969,22 +967,18 @@ impl ConjoiningClauses {
 
 impl ConjoiningClauses {
     pub fn apply_clauses(&mut self, schema: &Schema, where_clauses: Vec<WhereClause>) -> Result<()> {
-        let mut deferred = Vec::with_capacity(where_clauses.len());
         // We apply (top level) type predicates first as an optimization.
-        for c in where_clauses {
+        for clause in where_clauses.iter() {
-            match &c {
+            if let &WhereClause::TypeAnnotation(ref anno) = clause {
-                &WhereClause::Pred(ref p) => {
+                self.apply_type_anno(anno)?;
-                    if let Some(ty) = parse_type_predicate(p.operator.0.as_str()) {
+            }
-                        self.apply_type_requirement(p, ty)?;
-                    }
-                },
-                _ => {}
-            };
-            deferred.push(c);
         }
         // Then we apply everything else.
-        for c in deferred {
+        for clause in where_clauses {
-            self.apply_clause(schema, c)?;
+            if let &WhereClause::TypeAnnotation(_) = &clause {
+                continue;
+            }
+            self.apply_clause(schema, clause)?;
         }
         Ok(())
     }
@@ -1010,6 +1004,9 @@ impl ConjoiningClauses {
                 validate_not_join(&n)?;
                 self.apply_not_join(schema, n)
             },
+            WhereClause::TypeAnnotation(anno) => {
+                self.apply_type_anno(&anno)
+            },
             _ => unimplemented!(),
         }
     }

query-algebrizer/src/clauses/predicate.rs

@@ -17,6 +17,7 @@ use mentat_core::{
 use mentat_query::{
     FnArg,
     Predicate,
+    TypeAnnotation,
 };
 
 use clauses::ConjoiningClauses;
@@ -34,26 +35,11 @@ use types::{
     Inequality,
 };
 
-pub fn parse_type_predicate(s: &str) -> Option<ValueType> {
-    match s {
-        "ref" => Some(ValueType::Ref),
-        "boolean" => Some(ValueType::Boolean),
-        "instant" => Some(ValueType::Instant),
-        "long" => Some(ValueType::Long),
-        "double" => Some(ValueType::Double),
-        "string" => Some(ValueType::String),
-        "keyword" => Some(ValueType::Keyword),
-        "uuid" => Some(ValueType::Uuid),
-        _ => None
-    }
-}
-
 /// Application of predicates.
 impl ConjoiningClauses {
     /// There are several kinds of predicates in our Datalog:
     /// - A limited set of binary comparison operators: < > <= >= !=.
     ///   These are converted into SQLite binary comparisons and some type constraints.
-    /// - A set of type requirements constraining their argument to be a specific ValueType.
     /// - In the future, some predicates that are implemented via function calls in SQLite.
     ///
     /// At present we have implemented only the five built-in comparison binary operators.
@@ -62,8 +48,6 @@ impl ConjoiningClauses {
         // and ultimately allowing user-specified predicates, we match on the predicate name first.
         if let Some(op) = Inequality::from_datalog_operator(predicate.operator.0.as_str()) {
             self.apply_inequality(schema, op, predicate)
-        } else if let Some(ty) = parse_type_predicate(predicate.operator.0.as_str()) {
-            self.apply_type_requirement(&predicate, ty)
         } else {
             bail!(ErrorKind::UnknownFunction(predicate.operator.clone()))
         }
@@ -76,17 +60,11 @@ impl ConjoiningClauses {
         }
     }
 
-    pub fn apply_type_requirement(&mut self, pred: &Predicate, ty: ValueType) -> Result<()> {
+    /// Apply a type annotation, which is a construct like a predicate that constrains the argument
-        if pred.args.len() != 1 {
+    /// to be a specific ValueType.
-            bail!(ErrorKind::InvalidNumberOfArguments(pred.operator.clone(), pred.args.len(), 1));
+    pub fn apply_type_anno(&mut self, anno: &TypeAnnotation) -> Result<()> {
-        }
+        self.add_type_requirement(anno.variable.clone(), ValueTypeSet::of_one(anno.value_type));
-
+        Ok(())
-        if let &FnArg::Variable(ref v) = &pred.args[0] {
-            self.add_type_requirement(v.clone(), ValueTypeSet::of_one(ty));
-            Ok(())
-        } else {
-            bail!(ErrorKind::InvalidArgument(pred.operator.clone(), "variable".into(), 0))
-        }
     }
 
     /// This function:

query-parser/src/parse.rs

@@ -12,6 +12,7 @@ extern crate combine;
 extern crate edn;
 extern crate mentat_parser_utils;
 extern crate mentat_query;
+extern crate mentat_core;
 
 use std; // To refer to std::result::Result.
 
@@ -20,6 +21,8 @@ use std::collections::BTreeSet;
 use self::combine::{eof, many, many1, optional, parser, satisfy, satisfy_map, Parser, ParseResult, Stream};
 use self::combine::combinator::{any, choice, or, try};
 
+use self::mentat_core::ValueType;
+
 use self::mentat_parser_utils::{
     KeywordMapParser,
     ResultParser,
@@ -56,6 +59,7 @@ use self::mentat_query::{
     Predicate,
     QueryFunction,
     SrcVar,
+    TypeAnnotation,
     UnifyVars,
     Variable,
     VariableOrPlaceholder,
@@ -286,6 +290,44 @@ def_parser!(Where, pred, WhereClause, {
                 })))
 });
 
+def_parser!(Query, type_anno_type, ValueType, {
+    satisfy_map(|v: &edn::ValueAndSpan| {
+        match v.inner {
+            edn::SpannedValue::PlainSymbol(ref s) => {
+                let name = s.0.as_str();
+                match name {
+                    "ref" => Some(ValueType::Ref),
+                    "boolean" => Some(ValueType::Boolean),
+                    "instant" => Some(ValueType::Instant),
+                    "long" => Some(ValueType::Long),
+                    "double" => Some(ValueType::Double),
+                    "string" => Some(ValueType::String),
+                    "keyword" => Some(ValueType::Keyword),
+                    "uuid" => Some(ValueType::Uuid),
+                    _ => None
+                }
+            },
+            _ => None,
+        }
+    })
+});
+
+/// A type annotation.
+def_parser!(Where, type_annotation, WhereClause, {
+    // Accept either a nested list or a nested vector here:
+    // `[(string ?x)]` or `[[string ?x]]`
+    vector()
+        .of_exactly(seq()
+            .of_exactly((Query::type_anno_type(), Query::variable())
+                .map(|(ty, var)| {
+                    WhereClause::TypeAnnotation(
+                        TypeAnnotation {
+                            value_type: ty,
+                            variable: var,
+                        })
+                })))
+});
+
 /// A vector containing a parenthesized function expression and a binding.
 def_parser!(Where, where_fn, WhereClause, {
     // Accept either a nested list or a nested vector here:
@@ -356,6 +398,7 @@ def_parser!(Where, clause, WhereClause, {
             try(Where::not_join_clause()),
             try(Where::not_clause()),
 
+            try(Where::type_annotation()),
             try(Where::pred()),
             try(Where::where_fn()),
     ])
@@ -949,4 +992,21 @@ mod test {
                                                                  VariableOrPlaceholder::Variable(Variable::from_valid_name("?y"))]),
                               }));
     }
+
+    #[test]
+    fn test_type_anno() {
+        assert_edn_parses_to!(Where::type_annotation,
+                              "[(string ?x)]",
+                              WhereClause::TypeAnnotation(TypeAnnotation {
+                                  value_type: ValueType::String,
+                                  variable: Variable::from_valid_name("?x"),
+                              }));
+        assert_edn_parses_to!(Where::clause,
+                              "[[long ?foo]]",
+                              WhereClause::TypeAnnotation(TypeAnnotation {
+                                  value_type: ValueType::Long,
+                                  variable: Variable::from_valid_name("?foo"),
+                              }));
+
+    }
 }

query-sql/src/lib.rs

@@ -108,12 +108,12 @@ pub enum Constraint {
     },
     TypeCheck {
         value: ColumnOrExpression,
-        datatype: SQLDatatype
+        affinity: SQLTypeAffinity
     }
 }
 
 /// Type safe representation of the possible return values from `typeof`
-pub enum SQLDatatype {
+pub enum SQLTypeAffinity {
     Null, // "null"
     Integer, // "integer"
     Real, // "real"
@@ -326,14 +326,14 @@ impl QueryFragment for Op {
     }
 }
 
-impl QueryFragment for SQLDatatype {
+impl QueryFragment for SQLTypeAffinity {
     fn push_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult {
         out.push_sql(match *self {
-            SQLDatatype::Null => "'null'",
+            SQLTypeAffinity::Null => "'null'",
-            SQLDatatype::Integer => "'integer'",
+            SQLTypeAffinity::Integer => "'integer'",
-            SQLDatatype::Real => "'real'",
+            SQLTypeAffinity::Real => "'real'",
-            SQLDatatype::Text => "'text'",
+            SQLTypeAffinity::Text => "'text'",
-            SQLDatatype::Blob => "'blob'",
+            SQLTypeAffinity::Blob => "'blob'",
         });
         Ok(())
     }
@@ -394,11 +394,11 @@ impl QueryFragment for Constraint {
                 out.push_sql(")");
                 Ok(())
             },
-            &TypeCheck { ref value, ref datatype } => {
+            &TypeCheck { ref value, ref affinity } => {
                 out.push_sql("typeof(");
                 value.push_sql(out)?;
                 out.push_sql(") = ");
-                datatype.push_sql(out)?;
+                affinity.push_sql(out)?;
                 Ok(())
             },
         }

query-translator/src/translate.rs

@@ -51,7 +51,7 @@ use mentat_query_sql::{
     ProjectedColumn,
     Projection,
     SelectQuery,
-    SQLDatatype,
+    SQLTypeAffinity,
     TableList,
     TableOrSubquery,
     Values,
@@ -194,9 +194,9 @@ impl ToConstraint for ColumnConstraint {
                                 loose,
                                 Constraint::TypeCheck {
                                     value: val_column,
-                                    datatype: match ty {
+                                    affinity: match ty {
-                                        ValueType::Long => SQLDatatype::Integer,
+                                        ValueType::Long => SQLTypeAffinity::Integer,
-                                        ValueType::Double => SQLDatatype::Real,
+                                        ValueType::Double => SQLTypeAffinity::Real,
                                         _ => unreachable!()
                                     }
                                 }

query/src/lib.rs

@@ -56,6 +56,7 @@ pub use edn::{
 
 use mentat_core::{
     TypedValue,
+    ValueType,
 };
 
 pub type SrcVarName = String;          // Do not include the required syntactic '$'.
@@ -769,6 +770,12 @@ pub struct NotJoin {
     pub clauses: Vec<WhereClause>,
 }
 
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct TypeAnnotation {
+    pub value_type: ValueType,
+    pub variable: Variable,
+}
+
 #[allow(dead_code)]
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum WhereClause {
@@ -778,6 +785,7 @@ pub enum WhereClause {
     WhereFn(WhereFn),
     RuleExpr,
     Pattern(Pattern),
+    TypeAnnotation(TypeAnnotation),
 }
 
 #[allow(dead_code)]
@@ -852,12 +860,13 @@ impl ContainsVariables for WhereClause {
     fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
         use WhereClause::*;
         match self {
-            &OrJoin(ref o)  => o.accumulate_mentioned_variables(acc),
+            &OrJoin(ref o)         => o.accumulate_mentioned_variables(acc),
-            &Pred(ref p)    => p.accumulate_mentioned_variables(acc),
+            &Pred(ref p)           => p.accumulate_mentioned_variables(acc),
-            &Pattern(ref p) => p.accumulate_mentioned_variables(acc),
+            &Pattern(ref p)        => p.accumulate_mentioned_variables(acc),
-            &NotJoin(ref n) => n.accumulate_mentioned_variables(acc),
+            &NotJoin(ref n)        => n.accumulate_mentioned_variables(acc),
-            &WhereFn(ref f) => f.accumulate_mentioned_variables(acc),
+            &WhereFn(ref f)        => f.accumulate_mentioned_variables(acc),
-            &RuleExpr       => (),
+            &TypeAnnotation(ref a) => a.accumulate_mentioned_variables(acc),
+            &RuleExpr              => (),
         }
     }
 }
@@ -920,6 +929,13 @@ impl ContainsVariables for Predicate {
     }
 }
 
+
+impl ContainsVariables for TypeAnnotation {
+    fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
+        acc_ref(acc, &self.variable);
+    }
+}
+
 impl ContainsVariables for Binding {
     fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
         match self {

tests/query.rs

@@ -558,13 +558,11 @@ fn test_type_reqs() {
         "ref",
     ];
 
-    let entid_binding = QueryInputs::with_value_sequence(vec![
-        (Variable::from_valid_name("?e"), TypedValue::Ref(entid)),
-    ]);
-
     for name in type_names {
         let q = format!("[:find [?v ...] :in ?e :where [?e _ ?v] [({} ?v)]]", name);
-        let results = conn.q_once(&mut c, &q, entid_binding.clone()).unwrap();
+        let results = conn.q_once(&mut c, &q, QueryInputs::with_value_sequence(vec![
+            (Variable::from_valid_name("?e"), TypedValue::Ref(entid)),
+        ])).unwrap();
         match results {
             QueryResults::Coll(vals) => {
                 assert_eq!(vals.len(), 1, "Query should find exactly 1 item");
@@ -585,7 +583,9 @@ fn test_type_reqs() {
                           :in ?e
                           :where [?e _ ?v] [(long ?v)]]"#;
 
-    let res = conn.q_once(&mut c, longs_query, entid_binding.clone()).unwrap();
+    let res = conn.q_once(&mut c, longs_query, QueryInputs::with_value_sequence(vec![
+        (Variable::from_valid_name("?e"), TypedValue::Ref(entid)),
+    ])).unwrap();
     match res {
         QueryResults::Coll(vals) => {
             assert_eq!(vals, vec![TypedValue::Long(5), TypedValue::Long(33)])