From 7eea65b3e2ea742525060ef71421d7292086b418 Mon Sep 17 00:00:00 2001 From: Emily Toop Date: Thu, 6 Apr 2017 18:15:40 +0100 Subject: [PATCH] Address review comments rnewman. * Remove `WhereNotClause` and populate `NotJoin` with `WhereClause`. * Fix validation for `not` and `not-join`, removing tests that were invalid. * Address rustification comments. --- query-algebrizer/src/clauses/mod.rs | 2 +- query-algebrizer/src/validate.rs | 156 +++++++--------------------- query-parser/src/parse.rs | 22 ++-- query/src/lib.rs | 25 +---- 4 files changed, 48 insertions(+), 157 deletions(-) diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs index 054369ef..7083bea0 100644 --- a/query-algebrizer/src/clauses/mod.rs +++ b/query-algebrizer/src/clauses/mod.rs @@ -62,8 +62,8 @@ mod predicate; mod resolve; use validate::{ - validate_or_join, validate_not_join, + validate_or_join, }; // We do this a lot for errors. diff --git a/query-algebrizer/src/validate.rs b/query-algebrizer/src/validate.rs index 22d60c6a..864b4283 100644 --- a/query-algebrizer/src/validate.rs +++ b/query-algebrizer/src/validate.rs @@ -79,36 +79,13 @@ pub fn validate_not_join(not_join: &NotJoin) -> Result<()> { // Grab our mentioned variables and ensure that the rules are followed. match not_join.unify_vars { UnifyVars::Implicit => { - if not_join.clauses.len() < 2 { - Ok(()) - } else { - let mut clauses = not_join.clauses.iter(); - let template = clauses.next().unwrap().collect_mentioned_variables(); - for clause in clauses { - if template != clause.collect_mentioned_variables() { - bail!(ErrorKind::NonMatchingVariablesInNotClause); - } - } - Ok(()) - } + Ok(()) }, UnifyVars::Explicit(ref vars) => { // The joined vars must each appear somewhere in the clauses mentioned variables. let var_set: BTreeSet = vars.iter().cloned().collect(); - for var in &var_set { - if !¬_join.clauses.iter().any(|clause| clause.collect_mentioned_variables().contains(&var)) { - bail!(ErrorKind::NonMatchingVariablesInNotClause); - } - } - - // any other mentioned variables must also unify - let mut clauses = not_join.clauses.iter(); - let template: BTreeSet = clauses.next().unwrap().collect_mentioned_variables().difference(&var_set).cloned().collect(); - for clause in clauses { - let mentioned_vars = clause.collect_mentioned_variables().difference(&var_set).cloned().collect(); - if template != mentioned_vars { - bail!(ErrorKind::NonMatchingVariablesInOrClause); - } + if !var_set.is_subset(¬_join.collect_mentioned_variables()) { + bail!(ErrorKind::NonMatchingVariablesInNotClause); } Ok(()) }, @@ -125,7 +102,6 @@ mod tests { FindQuery, NamespacedKeyword, OrWhereClause, - WhereNotClause, Pattern, PatternNonValuePlace, PatternValuePlace, @@ -139,8 +115,8 @@ mod tests { use clauses::ident; use super::{ - validate_or_join, validate_not_join, + validate_or_join, }; fn value_ident(ns: &str, name: &str) -> PatternValuePlace { @@ -277,15 +253,15 @@ mod tests { /// Tests that the top-level form is a valid `not`, returning the clauses. - fn valid_not_join(parsed: FindQuery, expected_unify: UnifyVars) -> Vec { - // filter all the not clauses - let mut nots = parsed.where_clauses.iter().filter(|&x| match *x { - WhereClause::NotJoin(_) => true, + fn valid_not_join(parsed: FindQuery, expected_unify: UnifyVars) -> Vec { + // Filter out all the clauses that are not `not`s. + let mut nots = parsed.where_clauses.into_iter().filter(|x| match x { + &WhereClause::NotJoin(_) => true, _ => false, }); - // there should be only one not clause - let clause = nots.next().unwrap().clone(); + // There should be only one not clause. + let clause = nots.next().unwrap(); assert_eq!(None, nots.next()); match clause { @@ -299,7 +275,7 @@ mod tests { } } - /// Test that a `not` is valid if all of its arms refer to the same variables. + /// Test that a `not` is valid if it is implicit. #[test] fn test_success_not() { let query = r#"[:find ?name @@ -315,54 +291,27 @@ mod tests { (Some(clause1), Some(clause2), None) => { assert_eq!( clause1, - WhereNotClause::Clause( - WhereClause::Pattern(Pattern { - source: None, - entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?id")), - attribute: ident("artist", "country"), - value: value_ident("country", "CA"), - tx: PatternNonValuePlace::Placeholder, - }), - )); + WhereClause::Pattern(Pattern { + source: None, + entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?id")), + attribute: ident("artist", "country"), + value: value_ident("country", "CA"), + tx: PatternNonValuePlace::Placeholder, + })); assert_eq!( clause2, - WhereNotClause::Clause( - WhereClause::Pattern(Pattern { - source: None, - entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?id")), - attribute: ident("artist", "country"), - value: value_ident("country", "GB"), - tx: PatternNonValuePlace::Placeholder, - }), - )); + WhereClause::Pattern(Pattern { + source: None, + entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?id")), + attribute: ident("artist", "country"), + value: value_ident("country", "GB"), + tx: PatternNonValuePlace::Placeholder, + })); }, _ => panic!(), }; } - - /// Test that a `not` with differing variable sets in each body part will fail to validate. - #[test] - fn test_invalid_implicit_not() { - let query = r#"[:find ?name - :where [?id :artist/name ?name] - (not [?id :artist/country :country/CA] - [?release :release/year 1970])]"#; - let parsed = parse_find_string(query).expect("expected successful parse"); - let mut nots = parsed.where_clauses.iter().filter(|&x| match *x { - WhereClause::NotJoin(_) => true, - _ => false, - }); - - let clause = nots.next().unwrap().clone(); - assert_eq!(None, nots.next()); - - match clause { - WhereClause::NotJoin(not_join) => assert!(validate_not_join(¬_join).is_err()), - _ => panic!(), - } - } - #[test] fn test_success_not_join() { let query = r#"[:find ?artist @@ -373,30 +322,28 @@ mod tests { 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's do some detailed parse checks. + // Let's do some detailed parse checks. let mut parts = clauses.into_iter(); match (parts.next(), parts.next(), parts.next()) { (Some(clause1), Some(clause2), None) => { assert_eq!( clause1, - WhereNotClause::Clause( - WhereClause::Pattern(Pattern { - source: None, - entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?release")), - attribute: ident("release", "artists"), - value: PatternValuePlace::Variable(Variable::from_valid_name("?artist")), - tx: PatternNonValuePlace::Placeholder, - }))); + WhereClause::Pattern(Pattern { + source: None, + entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?release")), + attribute: ident("release", "artists"), + value: PatternValuePlace::Variable(Variable::from_valid_name("?artist")), + tx: PatternNonValuePlace::Placeholder, + })); assert_eq!( clause2, - WhereNotClause::Clause( - WhereClause::Pattern(Pattern { - source: None, - entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?release")), - attribute: ident("release", "year"), - value: PatternValuePlace::EntidOrInteger(1970), - tx: PatternNonValuePlace::Placeholder, - }))); + WhereClause::Pattern(Pattern { + source: None, + entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?release")), + attribute: ident("release", "year"), + value: PatternValuePlace::EntidOrInteger(1970), + tx: PatternNonValuePlace::Placeholder, + })); }, _ => panic!(), }; @@ -424,27 +371,4 @@ mod tests { _ => panic!(), } } - - /// Test that a `not-join` that does not match any vars declared inside it fails to validate. - #[test] - fn test_invalid_explicit_not_join_non_matching_internal_vars() { - let query = r#"[:find ?artist - :where [?artist :artist/name] - (not-join [?artist] - [?release :release/artists ?artist] - [?year :release/year 1970])]"#; - let parsed = parse_find_string(query).expect("expected successful parse"); - let mut nots = parsed.where_clauses.iter().filter(|&x| match *x { - WhereClause::NotJoin(_) => true, - _ => false, - }); - - let clause = nots.next().unwrap().clone(); - assert_eq!(None, nots.next()); - - match clause { - WhereClause::NotJoin(not_join) => assert!(validate_not_join(¬_join).is_err()), - _ => panic!(), - } - } } \ No newline at end of file diff --git a/query-parser/src/parse.rs b/query-parser/src/parse.rs index beee7a75..54bbcd05 100644 --- a/query-parser/src/parse.rs +++ b/query-parser/src/parse.rs @@ -43,7 +43,6 @@ use self::mentat_query::{ OrJoin, OrWhereClause, NotJoin, - WhereNotClause, Pattern, PatternNonValuePlace, PatternValuePlace, @@ -188,19 +187,11 @@ def_parser!(Where, or_join_clause, WhereClause, { })) }); -def_value_parser_fn!(Where, not_pattern_clause, WhereNotClause, input, { - Where::clause().map(|clause| WhereNotClause::Clause(clause)).parse_stream(input) -}); - -def_value_parser_fn!(Where, where_not_clause, WhereNotClause, input, { - choice([Where::not_pattern_clause()]).parse_stream(input) -}); - def_value_parser_fn!(Where, not_clause, WhereClause, input, { satisfy_map(|x: edn::Value| { seq(x).and_then(|items| { let mut p = Where::not() - .with(many1(Where::where_not_clause())) + .with(many1(Where::clause())) .skip(eof()) .map(|clauses| { WhereClause::NotJoin( @@ -220,7 +211,7 @@ def_value_parser_fn!(Where, not_join_clause, WhereClause, input, { seq(x).and_then(|items| { let mut p = Where::not_join() .with(Where::rule_vars()) - .and(many1(Where::where_not_clause())) + .and(many1(Where::clause())) .skip(eof()) .map(|(vars, clauses)| { WhereClause::NotJoin( @@ -618,14 +609,14 @@ mod test { WhereClause::NotJoin( NotJoin { unify_vars: UnifyVars::Implicit, - clauses: vec![WhereNotClause::Clause( + clauses: vec![ WhereClause::Pattern(Pattern { source: None, entity: PatternNonValuePlace::Variable(variable(e)), attribute: PatternNonValuePlace::Variable(variable(a)), value: PatternValuePlace::Variable(variable(v)), tx: PatternNonValuePlace::Placeholder, - }))], + })], })); } @@ -645,14 +636,13 @@ mod test { WhereClause::NotJoin( NotJoin { unify_vars: UnifyVars::Explicit(vec![variable(e.clone())]), - clauses: vec![WhereNotClause::Clause( - WhereClause::Pattern(Pattern { + clauses: vec![WhereClause::Pattern(Pattern { source: None, entity: PatternNonValuePlace::Variable(variable(e)), attribute: PatternNonValuePlace::Variable(variable(a)), value: PatternValuePlace::Variable(variable(v)), tx: PatternNonValuePlace::Placeholder, - }))], + })], })); } diff --git a/query/src/lib.rs b/query/src/lib.rs index ebea7376..27f8dfea 100644 --- a/query/src/lib.rs +++ b/query/src/lib.rs @@ -570,24 +570,10 @@ pub struct OrJoin { pub clauses: Vec, } -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum WhereNotClause { - Clause(WhereClause), -} - -impl WhereNotClause { - pub fn is_pattern_or_patterns(&self) -> bool { - match self { - &WhereNotClause::Clause(WhereClause::Pattern(_)) => true, - _ => false, - } - } -} - #[derive(Clone, Debug, Eq, PartialEq)] pub struct NotJoin { pub unify_vars: UnifyVars, - pub clauses: Vec, + pub clauses: Vec, } #[allow(dead_code)] @@ -680,15 +666,6 @@ impl ContainsVariables for NotJoin { } } -impl ContainsVariables for WhereNotClause { - fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet) { - use WhereNotClause::*; - match self { - &Clause(ref clause) => clause.accumulate_mentioned_variables(acc), - } - } -} - impl ContainsVariables for Predicate { fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet) { for arg in &self.args {