Allow customers to assert facts about the current transaction. by eoger · Pull Request #530 · mozilla/mentat

eoger · 2018-01-22T18:55:32Z

Fixes #225

rnewman · 2018-01-22T19:55:42Z

            partition_map: &'a PartitionMap,
            schema: &'a Schema,
            mentat_id_count: i64,
+            tx_id: Entid,


Use KnownEntid here.

rnewman · 2018-01-22T19:56:43Z

                        tx_might_update_metadata = true;
                    }

+                    if e == KnownEntid(self.tx_id) && a == entids::DB_TX_INSTANT {


You haven't checked that op == OpType::Add!

IMO, the simplest way to do all this work is to make self.tx_instant Option. That means we can get rid of tx_instant_set, skip doing the datom-producing work here, and always do the work down where we used to add txInstant by using unwrap_or.

rnewman · 2018-01-22T20:01:32Z

                    }

+                    if e == KnownEntid(self.tx_id) && a == entids::DB_TX_INSTANT {
+                        tx_instant_set = true;


Consider whether it is an error to have:

[:db/tx :db/txInstant #inst "2017-01-01T10:00:00Z"] [:db/tx :db/txInstant #inst "2017-01-01T22:00:00Z"]

rnewman · 2018-01-22T20:03:26Z

+                            panic!("This type error should have been caught earlier.");
+                        }
+                        // TODO: INSTANT to be strictly after the last transaction
+                        // timestamp and strictly before the current transactor timestamp.


We might want to weaken this condition — Datomic can assume correct clocks, but embedded clients cannot.

And we will need a way to bypass this, too, for syncing.

rnewman · 2018-01-22T20:05:44Z

+            non_fts_one.push((self.tx_id,
+                              entids::DB_TX_INSTANT,
+                              self.schema.require_attribute_for_entid(entids::DB_TX_INSTANT).unwrap(),
+                              TypedValue::Instant(self.tx_instant),


So this would be:

self.tx_instant.map_or_else(TypedValue::current_instant, TypedValue::Instant)

rnewman · 2018-01-22T20:07:35Z

        match self {
            TempId::Internal(x) => Some(x),
-            TempId::External(_) => None,
+            _ => None,


The trick you used to find these locations — add the enum case, look for the compile errors — stops working if you use _. Add the explicit TempId::Tx case here instead:

TempId::Internal(x) => Some(x), TempId::Tx | TempId::External(_) => None,

eoger · 2018-01-24T16:12:52Z

Thank you @rnewman, I have amended my patch.
Here is what I came up with regarding multiple values passed to txInstant:

[:db/add :db/tx :db/txInstant A]
[:db/add :db/tx :db/txInstant B]
// The last :db/add value is always picked up.
// Here txInstant = B.

[:db/add :db/tx :db/txInstant A]
[:db/add :db/tx :db/txInstant B]
[:db/retract :db/tx :db/txInstant B]
// If the last assertion about txInstant is a :db/retract and it corresponds to the last :db/add value, the system picks up a value.
// Here txInstant = ::now().

[:db/add :db/tx :db/txInstant A]
[:db/retract :db/tx :db/txInstant B]
// Here, txInstant = A.

rnewman · 2018-01-24T17:28:36Z

N.B., retractions need to pass through; they conceptually apply before the additions.

After working on #533 I think duplicate specifications within a single input should be an error; feel free to define a better error type ("duplicate datom") if one doesn't exist.

Fixes #225

eoger · 2018-01-29T20:30:11Z

Updated, thanks.
I removed most of the code that was in that for term in final_terms loop since #533 does the duplicate-detection heavy-lifting now.

rnewman · 2018-01-30T00:08:41Z

-                          self.schema.require_attribute_for_entid(entids::DB_TX_INSTANT).unwrap(),
-                          TypedValue::Instant(self.tx_instant),
-                          true));
+        tx_instant = self.tx_instant.unwrap_or(::now());


unwrap_or_else(TypeValue::current_instant).

Hm, no, the report returns a DateTime. My bad.

rnewman · 2018-01-30T00:09:52Z

+        tx_instant = self.tx_instant.unwrap_or(::now());
+
+        // Transact [:db/add :db/txInstant NOW :db/tx] if it doesn't exist.
+        if self.tx_instant == None {


You always set tx_instant right above this. I don't think you need this conditional check (or the comment).

We check self.tx_instant (not tx_instant) here, which is set conditionally in https://github.com/mozilla/mentat/pull/530/files#diff-f0d16e6d633d9a7ec44f52c5f064e75fR624

Yeah, but the point is: we've filtered out all txInstant-setting datoms from the input. Every tx is supposed to have one. So tx_instant (the local var) is the value we want, and we should always add it to the output. Right?

Oh, you stripped out the continue. Got it.

rnewman · 2018-01-30T00:50:04Z

3bf7459

rnewman suggested changes Jan 22, 2018

View reviewed changes

eoger force-pushed the eoger/tx-assertions branch from 87dd6a7 to e1ea288 Compare January 24, 2018 16:08

Allow customers to assert facts about the current transaction.

7cb44ad

Fixes #225

eoger force-pushed the eoger/tx-assertions branch from e1ea288 to 7cb44ad Compare January 29, 2018 20:28

rnewman suggested changes Jan 30, 2018

View reviewed changes

rnewman closed this Jan 30, 2018

rnewman deleted the eoger/tx-assertions branch January 30, 2018 00:50

Conversation

eoger commented Jan 22, 2018

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!

eoger commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnewman commented Jan 24, 2018

Uh oh!

eoger commented Jan 29, 2018

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 Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eoger commented Jan 24, 2018 •

edited

Loading