Crudely parse or and or-join. (#388)
#390
Conversation
rnewman
force-pushed
the
rnewman/or-join
branch
from
df264cd
to
23f03da
Compare
|
|
||
| NonMatchingVariablesInOrClause { | ||
| // TODO: flesh out. | ||
| description("non-matching variables in 'or' clause") |
There was a problem hiding this comment.
How does non-matching differ from unmatched? Or mismatched?
There was a problem hiding this comment.
I think you're asking about wording.
IMO there's not a lot of clarity in the distinction, if any, but:
- "Unmatched variables" kinda means we were looking for one or two and didn't get them.
- "Mismatched variables" means we were combining two things and they didn't match. A mismatch is bidirectional.
- "Non-matching variables" implies quite weakly that some expected property didn't hold.
That's the weak reasoning behind this wording: this error means that the expected property — that every arm in the or uses all of the variables in the rule vars, which might themselves be derived from the arms — doesn't hold. There can be an arbitrary number of arms.
query/src/lib.rs
Outdated
| @@ -515,3 +516,79 @@ pub struct FindQuery { | |||
| pub where_clauses: Vec<WhereClause>, | |||
| // TODO: in_rules; | |||
| } | |||
|
|
|||
| pub trait ContainsVariables { | |||
| fn acc_mentioned_variables(&self, acc: &mut BTreeSet<Variable>); | |||
There was a problem hiding this comment.
Consider accumulate_mentioned_variables.
There was a problem hiding this comment.
Or mentioned_variables, and then make the other function collect_mentioned_variables.
There was a problem hiding this comment.
I did both: accumulate_mentioned_variables, called by collect_mentioned_variables.
query-algebrizer/src/validate.rs
Outdated
| // Let's do some detailed parse checks. | ||
| let mut arms = clauses.into_iter(); | ||
| match (arms.next(), arms.next(), arms.next()) { | ||
| (Some(left), |
There was a problem hiding this comment.
nit: one line or different indentation. (Maybe this is an artifact of GH's layout, but it looks dedented one space.)
There was a problem hiding this comment.
Line 135 has a relative 4-space indent, but match is five characters long.
There was a problem hiding this comment.
Oh, I see — you were referring to 136. Fixed.
query-algebrizer/src/validate.rs
Outdated
| [?artist :artist/type ?type])]"#; | ||
| let parsed = parse_find_string(query).unwrap(); | ||
| match parsed.where_clauses.into_iter().next().unwrap() { | ||
| WhereClause::OrJoin(or_join) => assert!(validate_or_join(&or_join).is_err()), |
There was a problem hiding this comment.
This style can be quite difficult to debug when you break the tests, since you'll get unhelpful panics from the unwrap invocations.
There was a problem hiding this comment.
Switched for expects.
| } | ||
| } | ||
|
|
||
| /// Test that two arms of an `or-join` can contain different variables if they both |
There was a problem hiding this comment.
These descriptions really helped. Thanks!
query-algebrizer/src/lib.rs
Outdated
| }, | ||
| _ => unimplemented!(), | ||
| } | ||
| cc.apply_clause(schema, where_clause); |
There was a problem hiding this comment.
This ignores the result, doesn't it? You want the trailing ? or some other chaining operator.
There was a problem hiding this comment.
I'm surprised the compiler doesn't yell about this.
| }); | ||
|
|
||
| def_value_parser_fn!(Where, rule_vars, Vec<Variable>, input, { | ||
| satisfy_map(|x: edn::Value| { |
There was a problem hiding this comment.
It took a long time, but I think I've figured out improvements to this pattern, which should allow us to write something like:
or(vector(), list()).of(many1::<Vec<_>, _>(Query::variable())which I think is a significant improvement.
query/src/lib.rs
Outdated
| @@ -474,13 +474,30 @@ pub struct Predicate { | |||
| pub args: Vec<FnArg>, | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug, Eq, PartialEq)] | |||
| pub enum UnifyVars { | |||
| Implicit, | |||
There was a problem hiding this comment.
Worth a comment on the difference here. Am I correct that Implicit is equivalent to Explicit with the complete set of variables occurring in all (equivalently, each) clause?
There was a problem hiding this comment.
Yes, but — as far as I understand Datomic and DataScript, at any rate — implicit means that the extracted variables are all 'free', and explicit means that the specified join vars are all bound by the time the pattern is processed (and Datomic 'pushes down' until that's true).
So this is fine:
[:find ?x :where
(or [?x :foo/bar ?y]
[?x :foo/baz ?y])
[?y :foo/knows "John"]]
and this is fine:
[:find ?x :where
[?x :foo/name "John"]
(or-join [?x]
[?x :foo/friend ?y]
(and [?x :foo/friend ?z] [?z :foo/parent ?y]))]
I'm really unhappy about the amount of boilerplate in this parser. But it works.
Note that unlike DataScript's parser, this one doesn't validate the contents of the variable lists.