Support inequality predicates for instants #482

rnewman · 2017-06-16T01:44:36Z

This issue fixes #439.

#481 and #480 are fixes necessary for this to work correctly.

fluffyemily · 2017-06-16T14:05:36Z

query-algebrizer/src/types.rs

 /// Datalog and SQL don't use the same operators (e.g., `<>` and `!=`).
-pub enum NumericComparison {
+/// These are applicable to numbers and instants.
+pub enum Inequality {


Is Inequality entirely accurate when there are LessThanOrEquals and GreaterThanOrEquals as options on the enum?

Yes; the full technical term is "non-strict inequality".

fluffyemily · 2017-06-16T14:22:10Z

query-translator/tests/translate.rs

 }
+
+#[test]
+fn test_instant_range() {


Can we have another test please testing the self.mark_known_empty(EmptyBecause::NonInstantArgument); case?

This trait exposes potential_types, which returns the set of types to which the provided FnArg could conform.

Adds translation and end-to-end tests.

…re tests.

ncalexan

I'm basically OK with this, although I don't understand how the or case will be handled. Is it that the two inner CCs will each restrict their arguments based on the known types, and then the outer CC will do the right thing? What would happen if you injected the attribute name in via ground, so that the schema couldn't be queried to determine the type?

ncalexan · 2017-06-16T18:14:47Z

query-algebrizer/src/clauses/convert.rs

+                },
+
+                &FnArg::IdentOrKeyword(ref x) => {
+                    if schema.get_entid(x).is_some() {


Maybe a note that interacts with multi-stage algebrizing? I guess the &Schema parameter says that pretty clearly.

This bit in particular doesn't, except in the manner that all of our queries could fundamentally change behavior if the schema changes!

ncalexan · 2017-06-16T18:16:03Z

query-algebrizer/src/clauses/convert.rs

-                &FnArg::Constant(NonIntegerConstant::Instant(x)) => ValueTypeSet::of_one(ValueType::Instant),
-                &FnArg::Constant(NonIntegerConstant::Uuid(x)) => ValueTypeSet::of_one(ValueType::Uuid),
-                &FnArg::Constant(NonIntegerConstant::Float(x)) => ValueTypeSet::of_one(ValueType::Double),
+                &FnArg::Constant(NonIntegerConstant::Boolean(_)) => ValueTypeSet::of_one(ValueType::Boolean),


Fold into the previous patch.

ncalexan · 2017-06-16T18:19:42Z

query-algebrizer/src/clauses/predicate.rs

+                                     .intersection(&comparison.supported_types());
+
+        // We expect the intersection to be Long, Long+Double, Double, or Instant.
+        let left_v;


I didn't know Rust supported single-static-assignment with bare let. I wonder if there's a version requirement.

ncalexan · 2017-06-16T18:25:10Z

query-algebrizer/src/clauses/predicate.rs

+        // We expect the intersection to be Long, Long+Double, Double, or Instant.
+        let left_v;
+        let right_v;
+        if shared_types == ValueTypeSet::of_one(ValueType::Instant) {


Would it not be possible to ask for values that could be long or instants? Something like:

[:find ?x ?y :where (or (and [_ :some/instant ?x] [_ :some/other-instant ?y]) (and [_ :some/long ?x] [_ :some/other-long ?y]))]

I think what you mean is: can we algebrize a predicate where the types of the values are not known precisely, even if they could be mutually consistent?

I think your example needs a final clause for illustration:

[:find ?x ?y :where (or (and [_ :some/instant ?x] [_ :some/other-instant ?y]) (and [_ :some/long ?x] [_ :some/other-long ?y])) [(< ?x ?y)]]

The answer is: no, at least not yet. We don't maintain possibility trees, only type sets, so the best we know about your query is that ?x and ?y can each be either Instant or Long. That means it's possible — as far as the currently implementation is concerned, anyway — for ?x to be an Instant and ?y to be a Long, which we're not prepared to handle.

The simplest workaround is to push the condition into each branch:

[:find ?x ?y :where (or (and [_ :some/instant ?x] [_ :some/other-instant ?y] [(< ?x ?y)]) (and [_ :some/long ?x] [_ :some/other-long ?y] [(< ?x ?y)]))]

which will also likely be more efficient.

The other way of looking at your question is to realize that we have built ourselves this problem: the comparison operators in Clojure, and the comparisons in SQL, are overloaded. If the SQL comparison were DATE_LESS_THAN instead of <, then you wouldn't even be asking this question!

ncalexan · 2017-06-16T18:26:22Z

query-algebrizer/src/clauses/resolve.rs

+                Ok(QueryValue::TypedValue(TypedValue::Instant(v)))
+            },
+
+            // TODO: should we allow integers if they seem to be timestamps? It's ambiguous…


Generally, I'd prefer Mentat to be strict (with errors rather than empty result sets) in ambiguous cases.

ncalexan · 2017-06-16T18:30:30Z

query-algebrizer/tests/predicate.rs

+                    [?e :foo/date ?t]
+                    [(> ?t "2017-06-16T00:56:41.257Z")]]"#;
+    match bails(&schema, query).0 {
+        ErrorKind::InvalidArgument(op, why, idx) => {


Not feasible to derive Eq or PartialEq on ErrorKind, eh? Rats.

rust-lang-deprecated/error-chain#95
rust-lang-deprecated/error-chain#134

We're also waiting for that so we can derive Sync and upgrade to the current version of error_chain!

rnewman · 2017-06-16T18:59:18Z

eaf3e7f

rnewman added this to the Photino milestone Jun 16, 2017

rnewman self-assigned this Jun 16, 2017

rnewman requested review from fluffyemily and ncalexan June 16, 2017 01:44

fluffyemily approved these changes Jun 16, 2017

View reviewed changes

rnewman added 4 commits June 16, 2017 09:22

Pre: NumericInequality, NumericComparison -> Inequality.

e39578a

Pre: implement ValueTypes for FnArg.

23715bd

This trait exposes potential_types, which returns the set of types to which the provided FnArg could conform.

Extend inequalities to Instants. (#439)

672ed7b

Adds translation and end-to-end tests.

Much more thorough error handling, correct numeric comparison, add mo…

69addb3

…re tests.

rnewman force-pushed the rnewman/predicate-instants branch from de82eb2 to 69addb3 Compare June 16, 2017 18:22

ncalexan approved these changes Jun 16, 2017

View reviewed changes

rnewman closed this Jun 16, 2017

rnewman deleted the rnewman/predicate-instants branch June 16, 2017 18:59

rnewman mentioned this pull request Jun 16, 2017

[query] Support range predicates on instants #439

Closed

Support inequality predicates for instants #482

Support inequality predicates for instants #482

Uh oh!

Conversation

rnewman commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnewman commented Jun 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants