From c6e933c396dc1abe894f66c1e8df4a9a8c121304 Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Fri, 2 Jun 2017 15:36:12 -0700 Subject: [PATCH] Pre: make rule_vars return unique vars. --- query-algebrizer/src/clauses/not.rs | 2 +- query-algebrizer/src/clauses/or.rs | 2 +- query-algebrizer/src/lib.rs | 4 ++++ query-algebrizer/src/validate.rs | 6 ++--- query-parser/src/lib.rs | 3 +++ query-parser/src/parse.rs | 36 +++++++++++++++-------------- query-parser/tests/find_tests.rs | 7 ++++-- query/src/lib.rs | 2 +- 8 files changed, 37 insertions(+), 25 deletions(-) diff --git a/query-algebrizer/src/clauses/not.rs b/query-algebrizer/src/clauses/not.rs index 45723c51..2c2a57cf 100644 --- a/query-algebrizer/src/clauses/not.rs +++ b/query-algebrizer/src/clauses/not.rs @@ -32,7 +32,7 @@ impl ConjoiningClauses { pub fn apply_not_join(&mut self, schema: &Schema, not_join: NotJoin) -> Result<()> { let unified = match not_join.unify_vars { UnifyVars::Implicit => not_join.collect_mentioned_variables(), - UnifyVars::Explicit(vs) => vs.into_iter().collect(), + UnifyVars::Explicit(vs) => vs, }; let mut template = self.use_as_template(&unified); diff --git a/query-algebrizer/src/clauses/or.rs b/query-algebrizer/src/clauses/or.rs index e619cdfc..d4dfe2d9 100644 --- a/query-algebrizer/src/clauses/or.rs +++ b/query-algebrizer/src/clauses/or.rs @@ -552,7 +552,7 @@ impl ConjoiningClauses { let (join_clauses, unify_vars, mentioned_vars) = or_join.dismember(); let projected = match unify_vars { UnifyVars::Implicit => mentioned_vars.into_iter().collect(), - UnifyVars::Explicit(vs) => vs.into_iter().collect(), + UnifyVars::Explicit(vs) => vs, }; let template = self.use_as_template(&projected); diff --git a/query-algebrizer/src/lib.rs b/query-algebrizer/src/lib.rs index 504a6e14..9af4f0c2 100644 --- a/query-algebrizer/src/lib.rs +++ b/query-algebrizer/src/lib.rs @@ -13,6 +13,10 @@ extern crate enum_set; #[macro_use] extern crate error_chain; +#[cfg(test)] +#[macro_use] +extern crate maplit; + extern crate mentat_core; extern crate mentat_query; diff --git a/query-algebrizer/src/validate.rs b/query-algebrizer/src/validate.rs index 82fcc327..e5bb0df0 100644 --- a/query-algebrizer/src/validate.rs +++ b/query-algebrizer/src/validate.rs @@ -212,7 +212,7 @@ mod tests { (and [?artist :artist/type ?type] [?type :artist/role :artist.role/parody]))]"#; let parsed = parse_find_string(query).expect("expected successful parse"); - let clauses = valid_or_join(parsed, UnifyVars::Explicit(vec![Variable::from_valid_name("?artist")])); + let clauses = valid_or_join(parsed, UnifyVars::Explicit(btreeset!{Variable::from_valid_name("?artist")})); // Let's do some detailed parse checks. let mut arms = clauses.into_iter(); @@ -322,7 +322,7 @@ mod tests { [?release :release/artists ?artist] [?release :release/year 1970])]"#; let parsed = parse_find_string(query).expect("expected successful parse"); - let clauses = valid_not_join(parsed, UnifyVars::Explicit(vec![Variable::from_valid_name("?artist")])); + let clauses = valid_not_join(parsed, UnifyVars::Explicit(btreeset!{Variable::from_valid_name("?artist")})); let release = PatternNonValuePlace::Variable(Variable::from_valid_name("?release")); let artist = PatternValuePlace::Variable(Variable::from_valid_name("?artist")); @@ -375,4 +375,4 @@ mod tests { _ => panic!(), } } -} \ No newline at end of file +} diff --git a/query-parser/src/lib.rs b/query-parser/src/lib.rs index 8158d279..1c035276 100644 --- a/query-parser/src/lib.rs +++ b/query-parser/src/lib.rs @@ -10,6 +10,9 @@ #![allow(unused_imports)] +#[macro_use] +extern crate maplit; + #[macro_use] extern crate error_chain; diff --git a/query-parser/src/parse.rs b/query-parser/src/parse.rs index f5a58447..46f12751 100644 --- a/query-parser/src/parse.rs +++ b/query-parser/src/parse.rs @@ -197,9 +197,9 @@ def_matches_plain_symbol!(Where, not, "not"); def_matches_plain_symbol!(Where, not_join, "not-join"); -def_parser!(Where, rule_vars, Vec, { +def_parser!(Where, rule_vars, BTreeSet, { seq() - .of_exactly(many1(Query::variable())) + .of_exactly(many1(Query::variable()).and_then(unique_vars)) }); def_parser!(Where, or_pattern_clause, OrWhereClause, { @@ -392,18 +392,20 @@ def_parser!(Find, spec, FindSpec, { &mut try(Find::find_rel())]) }); +fn unique_vars(vars: Vec) -> std::result::Result, combine::primitives::Error> { + let given = vars.len(); + let set: BTreeSet = vars.into_iter().collect(); + if given != set.len() { + // TODO: find out what the variable is! + let e = Box::new(Error::from_kind(ErrorKind::DuplicateVariableError)); + Err(combine::primitives::Error::Other(e)) + } else { + Ok(set) + } +} + def_parser!(Find, vars, BTreeSet, { - many(Query::variable()).and_then(|vars: Vec| { - let given = vars.len(); - let set: BTreeSet = vars.into_iter().collect(); - if given != set.len() { - // TODO: find out what the variable is! - let e = Box::new(Error::from_kind(ErrorKind::DuplicateVariableError)); - Err(combine::primitives::Error::Other(e)) - } else { - Ok(set) - } - }) + many(Query::variable()).and_then(unique_vars) }); /// This is awkward, but will do for now. We use `keyword_map()` to optionally accept vector find @@ -573,7 +575,7 @@ mod test { let e = edn::PlainSymbol::new("?e"); let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone())]); assert_parses_to!(Where::rule_vars, input, - vec![variable(e.clone())]); + btreeset!{variable(e.clone())}); } #[test] @@ -583,7 +585,7 @@ mod test { let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone()), edn::Value::PlainSymbol(f.clone()),]); assert_parses_to!(|| vector().of_exactly(Find::vars()), input, - vec![variable(e.clone()), variable(f.clone())].into_iter().collect()); + btreeset!{variable(e.clone()), variable(f.clone())}); let g = edn::PlainSymbol::new("?g"); let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(g.clone()), @@ -642,7 +644,7 @@ mod test { edn::Value::PlainSymbol(v.clone())])].into_iter().collect()); assert_parses_to!(Where::or_join_clause, input, WhereClause::OrJoin( - OrJoin::new(UnifyVars::Explicit(vec![variable(e.clone())]), + OrJoin::new(UnifyVars::Explicit(btreeset!{variable(e.clone())}), vec![OrWhereClause::Clause( WhereClause::Pattern(Pattern { source: None, @@ -685,7 +687,7 @@ mod test { "(not-join [?e] [?e ?a ?v])", WhereClause::NotJoin( NotJoin { - unify_vars: UnifyVars::Explicit(vec![variable(e.clone())]), + unify_vars: UnifyVars::Explicit(btreeset!{variable(e.clone())}), clauses: vec![WhereClause::Pattern(Pattern { source: None, entity: PatternNonValuePlace::Variable(variable(e)), diff --git a/query-parser/tests/find_tests.rs b/query-parser/tests/find_tests.rs index be8ddd6c..d0873bce 100644 --- a/query-parser/tests/find_tests.rs +++ b/query-parser/tests/find_tests.rs @@ -8,6 +8,9 @@ // CONDITIONS OF ANY KIND, either express or implied. See the License for the // specific language governing permissions and limitations under the License. +#[macro_use] +extern crate maplit; + extern crate edn; extern crate mentat_core; extern crate mentat_query; @@ -111,7 +114,7 @@ fn can_parse_unit_or_join() { assert_eq!(p.where_clauses, vec![ WhereClause::OrJoin(OrJoin::new( - UnifyVars::Explicit(vec![Variable::from_valid_name("?x")]), + UnifyVars::Explicit(btreeset!{Variable::from_valid_name("?x")}), vec![ OrWhereClause::Clause( WhereClause::Pattern(Pattern { @@ -136,7 +139,7 @@ fn can_parse_simple_or_join() { assert_eq!(p.where_clauses, vec![ WhereClause::OrJoin(OrJoin::new( - UnifyVars::Explicit(vec![Variable::from_valid_name("?x")]), + UnifyVars::Explicit(btreeset!{Variable::from_valid_name("?x")}), vec![ OrWhereClause::Clause( WhereClause::Pattern(Pattern { diff --git a/query/src/lib.rs b/query/src/lib.rs index 414090f0..fd4f74f8 100644 --- a/query/src/lib.rs +++ b/query/src/lib.rs @@ -615,7 +615,7 @@ pub enum UnifyVars { /// Only the named variables will be unified with the enclosing query. /// /// Every 'arm' in an `or-join` must mention the entire set of explicit vars. - Explicit(Vec), + Explicit(BTreeSet), } impl WhereClause {