greg/mentat
1
0
Fork 0
Code Issues 294 Pull requests Packages Projects Releases 9 Wiki Activity

Handle :attribute/_reverse in transactor. Fixes #187

Browse source 
There are two broad approaches:

1) Handle reverse attribute notation dynamically, in the style that
   Datomic does.  This is the most flexible, but it's not a good fit
   given that we produce strongly typed output from the parser.
   Strongly typed input to the transactor has had many benefits, so I
   don't want to roll it back for a relatively unimportant feature
   like reverse notation -- especially not since Mentat does not
   require :db.install/_attribute to modify schema attributes.

2) Handle reverse attribute in the parser itself, so that we can
   produce strongly typed parser output while restricting the input.
   I implemented this first and discovered that it's very difficult to
   give sensible error messages in common cases.

In any case, the bulk of the code is the same between the two
approaches, and I wrote the tests for the dynamic version (with error
output), so that's what I'm rolling with.

This patch preserves the existing indentation, to highlight the
differences.  The next patch will indent.
This commit is contained in:
Nick Alexander 2017-06-06 15:21:51 -07:00
parent 0be78cf956
commit d88823e7c4
3 changed files with 218 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

123
db/src/db.rs
View file

@ -2087,4 +2087,127 @@ mod tests {
// dangling, if we give a :db/id explicitly.
assert_transact!(conn, "[{:test/dangling {:db/id \"t\" :test/many 11}}]");
}
#[test]
fn test_explode_reversed_notation() {
let mut conn = TestConn::default();
// Start by installing a few attributes.
assert_transact!(conn, "[[:db/add 111 :db/ident :test/many]
[:db/add 111 :db/valueType :db.type/long]
[:db/add 111 :db/cardinality :db.cardinality/many]
[:db/add 222 :db/ident :test/component]
[:db/add 222 :db/isComponent true]
[:db/add 222 :db/valueType :db.type/ref]
[:db/add 333 :db/ident :test/unique]
[:db/add 333 :db/unique :db.unique/identity]
[:db/add 333 :db/index true]
[:db/add 333 :db/valueType :db.type/long]
[:db/add 444 :db/ident :test/dangling]
[:db/add 444 :db/valueType :db.type/ref]]");
// Check that we can explode direct reversed notation, entids.
let report = assert_transact!(conn, "[[:db/add 100 :test/_dangling 200]]");
assert_matches!(conn.last_transaction(),
"[[200 :test/dangling 100 ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
assert_matches!(tempids(&report),
"{}");
// Check that we can explode direct reversed notation, idents.
let report = assert_transact!(conn, "[[:db/add :test/many :test/_dangling :test/unique]]");
assert_matches!(conn.last_transaction(),
"[[333 :test/dangling :test/many ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
assert_matches!(tempids(&report),
"{}");
// Check that we can explode direct reversed notation, tempids.
let report = assert_transact!(conn, "[[:db/add \"s\" :test/_dangling \"t\"]]");
assert_matches!(conn.last_transaction(),
"[[65537 :test/dangling 65536 ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
// This is implementation specific, but it should be deterministic.
assert_matches!(tempids(&report),
"{\"s\" 65536
\"t\" 65537}");
// Check that we can explode reversed notation in map notation without :db/id.
let report = assert_transact!(conn, "[{:test/_dangling 501}
{:test/_dangling :test/many}
{:test/_dangling \"t\"}]");
assert_matches!(conn.last_transaction(),
"[[111 :test/dangling ?e1 ?tx true]
[501 :test/dangling ?e2 ?tx true]
[65538 :test/dangling ?e3 ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
assert_matches!(tempids(&report),
"{\"t\" 65538}");
// Check that we can explode reversed notation in map notation with :db/id, entid.
let report = assert_transact!(conn, "[{:db/id 600 :test/_dangling 601}]");
assert_matches!(conn.last_transaction(),
"[[601 :test/dangling 600 ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
assert_matches!(tempids(&report),
"{}");
// Check that we can explode reversed notation in map notation with :db/id, ident.
let report = assert_transact!(conn, "[{:db/id :test/component :test/_dangling :test/component}]");
assert_matches!(conn.last_transaction(),
"[[222 :test/dangling :test/component ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
assert_matches!(tempids(&report),
"{}");
// Check that we can explode reversed notation in map notation with :db/id, tempid.
let report = assert_transact!(conn, "[{:db/id \"s\" :test/_dangling \"t\"}]");
assert_matches!(conn.last_transaction(),
"[[65543 :test/dangling 65542 ?tx true]
[?tx :db/txInstant ?ms ?tx true]]");
// This is implementation specific, but it should be deterministic.
assert_matches!(tempids(&report),
"{\"s\" 65542
\"t\" 65543}");
// Verify that we can't explode direct reverse notation with nested value maps.
assert_transact!(conn,
"[[:db/add \"t\" :test/_dangling {:test/many 11}]]",
Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444"));
// Verify that we can't explode reverse notation in map notation with nested value maps.
assert_transact!(conn,
"[{:test/_dangling {:test/many 11}}]",
Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444"));
// Verify that we can't explode direct reverse notation with nested value vectors.
assert_transact!(conn,
"[[:db/add \"t\" :test/_dangling [:test/many]]]",
Err("not yet implemented: Cannot explode vector value in :attr/_reversed notation for attribute 444"));
// Verify that we can't explode reverse notation in map notation with nested value vectors.
assert_transact!(conn,
"[{:test/_dangling [:test/many]}]",
Err("not yet implemented: Cannot explode vector value in :attr/_reversed notation for attribute 444"));
// Verify that we can't use reverse notation with non-:db.type/ref attributes.
assert_transact!(conn,
"[{:test/_unique 500}]",
Err("not yet implemented: Cannot use :attr/_reversed notation for attribute 333 that is not :db/valueType :db.type/ref"));
// Verify that we can't use reverse notation with unrecognized attributes.
assert_transact!(conn,
"[{:test/_unknown 500}]",
Err("no entid found for ident: :test/unknown")); // TODO: make this error reference the original :test/_unknown.
// Verify that we can't use reverse notation with bad value types: here, an unknown keyword
// that can't be coerced to a ref.
assert_transact!(conn,
"[{:test/_dangling :test/unknown}]",
Err("no entid found for ident: :test/unknown"));
// And here, a float.
assert_transact!(conn,
"[{:test/_dangling 1.23}]",
Err("EDN value \'1.23\' is not the expected Mentat value type Ref"));
}
}

90
db/src/tx.rs
View file

@ -269,6 +269,53 @@ impl<'conn, 'a> Tx<'conn, 'a> {
fn entity_e_into_term_v(&mut self, x: entmod::EntidOrLookupRefOrTempId) -> Result<TypedValueOr<LookupRefOrTempId>> {
self.entity_e_into_term_e(x).map(|r| r.map_left(TypedValue::Ref))
}
fn entity_v_into_term_e(&mut self, x: entmod::AtomOrLookupRefOrVectorOrMapNotation, backward_a: &entmod::Entid) -> Result<EntidOr<LookupRefOrTempId>> {
let reversed_a = match backward_a {
&entmod::Entid::Entid(a) => {
// Programmer error.
unreachable!("Expected ident backward attribute but got entid {}", a)
},
&entmod::Entid::Ident(ref a) => {
entmod::Entid::Ident(a.to_reversed())
},
};
let (reversed_a, reversed_attribute) = self.entity_a_into_term_a(reversed_a)?;
if reversed_attribute.value_type != ValueType::Ref {
bail!(ErrorKind::NotYetImplemented(format!("Cannot use :attr/_reversed notation for attribute {} that is not :db/valueType :db.type/ref", reversed_a)))
}
match x {
entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(ref v) => {
// Here is where we do schema-aware typechecking: we either assert
// that the given value is in the attribute's value set, or (in
// limited cases) coerce the value into the attribute's value set.
if let Some(text) = v.inner.as_text() {
Ok(Either::Right(LookupRefOrTempId::TempId(self.intern_temp_id(TempId::External(text.clone())))))
} else {
// Here is where we do schema-aware typechecking: we either assert that
// the given value is in the attribute's value set, or (in limited
// cases) coerce the value into the attribute's value set.
if let TypedValue::Ref(entid) = self.schema.to_typed_value(&v.clone().without_spans(), &reversed_attribute)? {
Ok(Either::Left(entid))
} else {
// reversed_attribute is :db.type/ref, so this shouldn't happen.
unreachable!()
}
}
},
entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(ref lookup_ref) =>
Ok(Either::Right(LookupRefOrTempId::LookupRef(self.intern_lookup_ref(lookup_ref)?))),
entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(_) =>
bail!(ErrorKind::NotYetImplemented(format!("Cannot explode vector value in :attr/_reversed notation for attribute {}", reversed_a))),
entmod::AtomOrLookupRefOrVectorOrMapNotation::MapNotation(_) =>
bail!(ErrorKind::NotYetImplemented(format!("Cannot explode map notation value in :attr/_reversed notation for attribute {}", reversed_a))),
}
}
}
let mut in_process = InProcess::with_schema(&self.schema);
@ -301,7 +348,13 @@ impl<'conn, 'a> Tx<'conn, 'a> {
},
Entity::AddOrRetract { op, e, a, v } => {
let (a, attribute) = in_process.entity_a_into_term_a(a)?;
if a.is_backward() {
let reversed_e = in_process.entity_v_into_term_e(v, &a)?;
let (reversed_a, _reversed_attribute) = in_process.entity_a_into_term_a(a.to_reversed().unwrap())?;
let reversed_v = in_process.entity_e_into_term_v(e)?;
terms.push(Term::AddOrRetract(OpType::Add, reversed_e, reversed_a, reversed_v));
} else {
let (a, attribute) = in_process.entity_a_into_term_a(a)?;
let v = match v {
entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => {
@ -373,18 +426,32 @@ impl<'conn, 'a> Tx<'conn, 'a> {
}
for (inner_a, inner_v) in map_notation {
let (inner_a, inner_attribute) = in_process.entity_a_into_term_a(inner_a)?;
if inner_attribute.unique == Some(attribute::Unique::Identity) {
if inner_a.is_backward() {
// We definitely have a reference. The reference might be
// dangling (a bare entid, for example), but we don't yet
// support nested maps and reverse notation simultaneously
// (i.e., we don't accept {:reverse/_attribute {:nested map}})
// so we don't need to check that the nested map reference isn't
// danglign.
dangling = false;
}
deque.push_front(Entity::AddOrRetract {
op: OpType::Add,
e: db_id.clone(),
a: entmod::Entid::Entid(inner_a),
v: inner_v,
});
let reversed_e = in_process.entity_v_into_term_e(inner_v, &inner_a)?;
let (reversed_a, _reversed_attribute) = in_process.entity_a_into_term_a(inner_a.to_reversed().unwrap())?;
let reversed_v = in_process.entity_e_into_term_v(db_id.clone())?;
terms.push(Term::AddOrRetract(OpType::Add, reversed_e, reversed_a, reversed_v));
} else {
let (inner_a, inner_attribute) = in_process.entity_a_into_term_a(inner_a)?;
if inner_attribute.unique == Some(attribute::Unique::Identity) {
dangling = false;
}
deque.push_front(Entity::AddOrRetract {
op: OpType::Add,
e: db_id.clone(),
a: entmod::Entid::Entid(inner_a),
v: inner_v,
});
}
}
if dangling {
@ -397,6 +464,7 @@ impl<'conn, 'a> Tx<'conn, 'a> {
let e = in_process.entity_e_into_term_e(e)?;
terms.push(Term::AddOrRetract(op, e, a, v));
}
},
}
};

16
tx/src/entities.rs
View file

@ -56,6 +56,22 @@ pub enum Entid {
Ident(NamespacedKeyword),
}
impl Entid {
pub fn is_backward(&self) -> bool {
match self {
&Entid::Entid(_) => false,
&Entid::Ident(ref a) => a.is_backward(),
}
}
pub fn to_reversed(&self) -> Option<Entid> {
match self {
&Entid::Entid(_) => None,
&Entid::Ident(ref a) => Some(Entid::Ident(a.to_reversed())),
}
}
}
#[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
pub struct LookupRef {
pub a: Entid,