Pull improvements #682
Pull improvements #682
Conversation
| @@ -303,11 +305,28 @@ def_parser!(Query, aggregate, Aggregate, { | |||
| }) | |||
| }); | |||
|
|
|||
| def_parser!(Query, pull_concrete_attribute_ident, PullConcreteAttribute, { | |||
There was a problem hiding this comment.
I would have expected to be expressed as an optional :as foo, but I guess this works as well. The multiple try blocks should be a little less efficient than the optional block, I think.
There was a problem hiding this comment.
Yep — Datomic's grammar is wrong. I will fix.
query-pull/src/lib.rs
Outdated
| .map(|ref r| r.to_value_rc()); | ||
| match attribute { | ||
| &PullConcreteAttribute::Ident(ref i) => { | ||
| if let Some(entid) = schema.get_entid(i) { |
There was a problem hiding this comment.
Again, I think this should fail if we can't look up the entid for an ident, since it's probably an error to pull a non-attribute.
query/src/lib.rs
Outdated
| // PullMapSpec(Vec<…>), | ||
| // AttributeWithOpts(PullConcreteAttribute, …), | ||
| // LimitedAttribute(PullConcreteAttribute, u64), // Limit nil => Attribute instead. |
There was a problem hiding this comment.
PullConcreteAttribute has been superceded, so update these comments to NamedPullAttribute.
| @@ -67,6 +67,7 @@ use self::mentat_query::{ | |||
| Order, | |||
| OrJoin, | |||
| OrWhereClause, | |||
| NamedPullAttribute, | |||
There was a problem hiding this comment.
I'd vaguely prefer Aliased to Named, since that's what we're using throughout, but I'm not hung up on it.
There was a problem hiding this comment.
I agreed with you, but it turns out that these are not always aliases; this is simply a pull attribute and its name, which sometimes differs from the attribute keyword itself.
query-pull/src/lib.rs
Outdated
| @@ -143,6 +145,7 @@ pub struct Puller { | |||
| // The range is the set of aliases to use in the output. | |||
| attributes: BTreeMap<Entid, ValueRc<NamespacedKeyword>>, | |||
| attribute_spec: cache::AttributeSpec, | |||
| db_id_alias: Option<ValueRc<NamespacedKeyword>>, | |||
There was a problem hiding this comment.
Definitely wants a comment here, since this is a) not Datomic-alike and b) subtle.
| @@ -22,6 +22,11 @@ error_chain! { | |||
| description("unnamed attribute") | |||
| display("attribute {:?} has no name", id) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The commit message is missing a "be": "to BE mentioned as a pull attribute."
| // TODO: limits. | ||
| // TODO: fts. |
There was a problem hiding this comment.
I didn't see any obvious FTS extractions in the tests. Are there some?
|
Rebased on top of #689: I'm about to add non-namespaced aliases, but it'll have to wait for Monday! |
This PR allows aliasing of pull expressions and the use of
:db/idfor the entity ID itself:I wanted to flatten
NamespacedKeyword, but soon realized that #648 still hasn't been wrapped up and landed, so I'll do that tomorrow. I plan to mergeNamespacedKeywordandKeyword, with the only difference being a non-zero division offset and a different result from a predicate; we can preserve the restriction that all attribute names are namespaced while normalizing the API. That will make aliasing more useful.