From 6584f21a7ec7aa9bc3e6e41d386967d338e623f6 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Fri, 26 Jan 2018 18:41:14 -0500 Subject: [PATCH] Address review feedback unrelated to the translator --- query-algebrizer/src/clauses/inputs.rs | 1 - query-algebrizer/src/clauses/mod.rs | 27 +++++----- query-algebrizer/src/clauses/predicate.rs | 34 +++---------- query-parser/src/parse.rs | 60 +++++++++++++++++++++++ query-sql/src/lib.rs | 20 ++++---- query-translator/src/translate.rs | 8 +-- query/src/lib.rs | 28 ++++++++--- tests/query.rs | 12 ++--- 8 files changed, 120 insertions(+), 70 deletions(-) diff --git a/query-algebrizer/src/clauses/inputs.rs b/query-algebrizer/src/clauses/inputs.rs index f3d8f9f6..4644fc4d 100644 --- a/query-algebrizer/src/clauses/inputs.rs +++ b/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, diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs index 3b1a5c6e..a71c4c32 100644 --- a/query-algebrizer/src/clauses/mod.rs +++ b/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) -> 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 { - match &c { - &WhereClause::Pred(ref p) => { - if let Some(ty) = parse_type_predicate(p.operator.0.as_str()) { - self.apply_type_requirement(p, ty)?; - } - }, - _ => {} - }; - deferred.push(c); + for clause in where_clauses.iter() { + if let &WhereClause::TypeAnnotation(ref anno) = clause { + self.apply_type_anno(anno)?; + } } // Then we apply everything else. - for c in deferred { - self.apply_clause(schema, c)?; + for clause in where_clauses { + 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!(), } } diff --git a/query-algebrizer/src/clauses/predicate.rs b/query-algebrizer/src/clauses/predicate.rs index ae89ec8e..de6fc66d 100644 --- a/query-algebrizer/src/clauses/predicate.rs +++ b/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 { - 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<()> { - if pred.args.len() != 1 { - bail!(ErrorKind::InvalidNumberOfArguments(pred.operator.clone(), pred.args.len(), 1)); - } - - 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)) - } + /// Apply a type annotation, which is a construct like a predicate that constrains the argument + /// to be a specific ValueType. + pub fn apply_type_anno(&mut self, anno: &TypeAnnotation) -> Result<()> { + self.add_type_requirement(anno.variable.clone(), ValueTypeSet::of_one(anno.value_type)); + Ok(()) } /// This function: diff --git a/query-parser/src/parse.rs b/query-parser/src/parse.rs index f1250fc8..0a4ad37d 100644 --- a/query-parser/src/parse.rs +++ b/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"), + })); + + } } diff --git a/query-sql/src/lib.rs b/query-sql/src/lib.rs index f313c8dc..8bcee6a6 100644 --- a/query-sql/src/lib.rs +++ b/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'", - SQLDatatype::Integer => "'integer'", - SQLDatatype::Real => "'real'", - SQLDatatype::Text => "'text'", - SQLDatatype::Blob => "'blob'", + SQLTypeAffinity::Null => "'null'", + SQLTypeAffinity::Integer => "'integer'", + SQLTypeAffinity::Real => "'real'", + SQLTypeAffinity::Text => "'text'", + 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(()) }, } diff --git a/query-translator/src/translate.rs b/query-translator/src/translate.rs index aceb9dd3..a361c22f 100644 --- a/query-translator/src/translate.rs +++ b/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 { - ValueType::Long => SQLDatatype::Integer, - ValueType::Double => SQLDatatype::Real, + affinity: match ty { + ValueType::Long => SQLTypeAffinity::Integer, + ValueType::Double => SQLTypeAffinity::Real, _ => unreachable!() } } diff --git a/query/src/lib.rs b/query/src/lib.rs index 7271d81e..a494734e 100644 --- a/query/src/lib.rs +++ b/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, } +#[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) { use WhereClause::*; match self { - &OrJoin(ref o) => o.accumulate_mentioned_variables(acc), - &Pred(ref p) => p.accumulate_mentioned_variables(acc), - &Pattern(ref p) => p.accumulate_mentioned_variables(acc), - &NotJoin(ref n) => n.accumulate_mentioned_variables(acc), - &WhereFn(ref f) => f.accumulate_mentioned_variables(acc), - &RuleExpr => (), + &OrJoin(ref o) => o.accumulate_mentioned_variables(acc), + &Pred(ref p) => p.accumulate_mentioned_variables(acc), + &Pattern(ref p) => p.accumulate_mentioned_variables(acc), + &NotJoin(ref n) => n.accumulate_mentioned_variables(acc), + &WhereFn(ref f) => f.accumulate_mentioned_variables(acc), + &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) { + acc_ref(acc, &self.variable); + } +} + impl ContainsVariables for Binding { fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet) { match self { diff --git a/tests/query.rs b/tests/query.rs index 2cdb6584..2f27fd7e 100644 --- a/tests/query.rs +++ b/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)])