Schema alteration. by ncalexan · Pull Request #370 · mozilla/mentat

ncalexan · 2017-03-10T16:36:49Z

This is WIP towards #294 and #295 and the equivalent for :db/ident. Not quite ready for review yet.

rnewman · 2017-03-11T00:55:25Z

+use edn::NamespacedKeyword;
+
+impl Schema {
+    pub fn get_ident(&self, x: Entid) -> Option<&NamespacedKeyword> {


For the sake of thoroughness, can we mark all of these as #[inline]?

rnewman · 2017-03-11T00:56:38Z

+    /// Associate the given ident with given entid if neither are already recognized.
+    ///
+    /// Return `true` if an association was added, `false` otherwise.
+    pub fn associate_ident(&mut self, i: NamespacedKeyword, e: Entid) -> bool {


Did you consider returning Result<()>? Result has the advantage of forcing the caller to check the return value…

If I associate an ident with an entid — which I can do, 'cos this is public — that hasn't already been allocated, all hell will break loose, right?

schema.associate_ident(NamespacedKeyword::new("foo", "bar"), 666); // Now transact ~600 tempids… one of them will magically be given the ident :foo/bar!

Can you validate here that the entid is within the existing part range? Or document that the caller should ensure that it is?

rnewman · 2017-03-11T01:01:24Z

+    pub fn associate_ident(&mut self, i: NamespacedKeyword, e: Entid) -> bool {
+        use std::collections::btree_map::Entry;
+        match (self.entid_map.entry(e), self.ident_map.entry(i.clone())) {
+            (Entry::Vacant(entid_entry), Entry::Vacant(ident_entry)) => {


You can get ever so slightly better performance via:

if let Entry::Vacant(entid_entry) = self.entid_map.entry(e) { if let Entry::Vacant(…) = … { entid_entry.insert(i); ident_entry.insert(e); return true; } } false

— don't bother looking up the second if the first isn't vacant.

rnewman · 2017-03-11T01:03:56Z

+    pub fn update_ident(&mut self, i: NamespacedKeyword, e: Entid) -> bool {
+        use std::collections::btree_map::Entry;
+        if let Entry::Occupied(mut entid_entry) = self.entid_map.entry(e) {
+            // TODO: avoid clone?


To avoid the clone here you can use the return value of remove to decide whether to return false. You don't do anything with the second entry beyond making sure that it's occupied before removing it; remove returns None if there was nothing to remove.

rnewman · 2017-03-11T01:04:50Z

+
+lazy_static! {
+    pub static ref ATTRIBUTE_SET: String = {
+        // let mut stmt = self.prepare_cached("SELECT e, a, v, value_type_tag, added FROM transactions WHERE tx = ? AND a IN 


rnewman · 2017-03-11T01:14:53Z

+}
+
+impl AttributeBuilder {
+    pub fn value_type<'a>(&'a mut self, value_type: ValueType) -> &'a mut Self {


Why mutable references rather than ownership? Surely the model is

let foo = AttributeBuilder::new().value_type(ValueType::Boolean).build();

which can be modeled as

pub fn value_type(self, value_type: ValueType) -> Self {

no?

This is not being consumed in a fluent interface (like Builder::new().thing1().thing2().build()), but in a loop that grabs the builder reference, does a thing, and then finally builds everything at the end. It's equivalent to consume self and update the &mut Builder in the map entry, I guess.

rnewman · 2017-03-11T01:16:38Z

+
+/// Return `false` if the given reduced entity will not change the metadata: recognized idents,
+/// schema, partitions in the partition map.
+pub fn might_update_metadata(reduced: &db::ReducedEntity) -> bool {


I'd suggest marking this as #[inline], see if it speeds up the transactor tight loop.

rnewman · 2017-03-11T01:19:57Z

+        // Idents.
+        entids::DB_IDENT |
+        // Schema.
+        entids::DB_ALTER_ATTRIBUTE |


Could you put these in numeric order, with the numeric value from entids.rs alongside?

I'd also be interested in an optimization here: hard-code the range in entids.rs (1–35, I think), and bail early if the value isn't in that range. No point painstakingly comparing 220 to 11 different small integers only to return false. The vast majority of assertions won't be in this range, so I'm keen to make this function ~free in the common case.

I'd be happy to optimize this eventually. I believe that Rust implements this via a jump table in some form, so it's ~free already.

rnewman · 2017-03-11T01:21:03Z

+
+    for (e, a, typed_value, added) in assertions.into_iter() {
+        let attribute = schema.require_attribute_for_entid(a)?;
+        // XXX TODO NOTE


rnewman · 2017-03-11T01:23:24Z


                    let added = op == OpType::Add;
+                    let reduced = (e, a, attribute, v, added);
+                    if metadata::might_update_metadata(&reduced) {


This function doesn't use most of the arguments. Can you just pass e by value instead?

https://medium.com/@robertgrosse/how-copying-an-int-made-my-code-11-times-faster-f76c66312e0f#.m3xlziaht

ncalexan · 2017-03-15T04:14:41Z

@rnewman OK, this is ready for review. This shares very little code with what you saw before; I hope I captured the spirit (or explicitly addressed) your major concerns, but I apologize if I'm missing stuff in the churn. I've been working on this too long without landing pieces, for which I apologise. Turned out to be a complex thing to get right.

rnewman

I'm not entirely sure I understand your division of entids vs idents — I would expect both idents and entids to be scoped to current data, and the for lookup in either direction to be a partial injective function. As I understand that part, you've made keyword to entid partial and injective, but entid to keyword partial and multivalued Am I misreading that commit message?

rnewman · 2017-03-15T09:19:15Z

-        let typed_value = TypedValue::from_sql_value_pair(v, value_type_tag)?;
+        let typed_value = match TypedValue::from_sql_value_pair(v, value_type_tag)? {
+            TypedValue::Ref(e) => {
+                borrowed_schema.get_ident(e).cloned().map(TypedValue::Keyword).unwrap_or(TypedValue::Ref(e))


You do this twice, so why not add a trait to TypedValue that ident-maps with a provided schema?

trait ToIdent { fn map_ident(self, schema: &Schema) -> Self; }

rnewman · 2017-03-15T09:22:02Z

+            }
+        } else {
+            if let Some(asserted_value) = self.asserted.remove(&key) {
+                self.altered.insert(key, (value, asserted_value));


I don't understand this. If there was an asserted value, and we get a removal, store an alteration for the original value instead?

Oh, I see. If you get the addition before the retraction, retract the old value and assert the new one.

rnewman · 2017-03-15T09:27:00Z

+/// Witness assertions and retractions, folding (assertion, retraction) pairs into alterations.
+/// Assumes that no assertion or retraction will be witnessed more than once.
+#[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
+pub struct DiffSet<K, V> {


This looks like a much more general data structure than it actually is! I was having trouble reasoning about what happens in some orderings, what happens if there is or isn't assumed to be data outside… but all it really does is filter out and categorize pairs, so given [assert(2), retract(1)] you don't get [], you get [alter(1, 2)].

Could you add a little more commentary and/or a test to explain what happens for various inputs?

Correct. I will certainly elaborate and add a few tests. It was just a thing I needed twice -- for attributes and for idents. Feel free to suggest another name, if you prefer.

AddRetractAlterSet?

rnewman · 2017-03-15T09:35:59Z

+    }
+
+    for &entid in &metadata_report.attributes_installed {
+        conn.execute(format!("INSERT INTO schema SELECT e, a, v, value_type_tag FROM datoms WHERE e = ? AND a IN {}", entids::SCHEMA_SET.as_str()).as_str(),


Prepared statements inside loops, please!

In general, I'm for cached statements (especially since Rusqlite makes them easy to use) but I intentionally didn't optimize this process. I expect it to be infrequent ... oh, prepared vs. cached. Prepared I will do! This is not efficient at this time because walking transactions in this way is very expensive but it's easy to improve and I'll file follow-up to track that.

rnewman · 2017-03-15T09:47:25Z

Sorry, in that comment I reversed the directions; I think you get my gist, though.

With historical idents stored in the schema, how do we recognize the current mapping?

ncalexan · 2017-03-15T16:09:18Z

I'm not entirely sure I understand your division of entids vs idents — I would expect both idents and entids to be scoped to current data, and the for lookup in either direction to be a partial injective function. As I understand that part, you've made keyword to entid partial and injective, but entid to keyword partial and multivalued Am I misreading that commit message?

I think you are. I think the following example will be instructive, and I'll fold it into a comment or commit message once we have shared understanding. Suppose I transact a single ident:

[[:db/add 100 :db/ident :test/ident]]

Then both the "entids" and "idents" materialized views will look (symbolically) like:

[[100 :db/ident :test/ident]]

If I now transact the following datoms:

[[:db/retract 100 :db/ident :test/ident]]
 [:db/add 100 :db/ident :test/updated]]

the "entids" materialized view will look like:

[[100 :db/ident :test/updated]]

but the "idents" materialized view will maintain the historical values, like:

[[100 :db/ident :test/ident]
 [100 :db/ident :test/updated]]

In terms of the resulting maps, entid_map will be {100 :test/ident} and ident_map will be {:test/ident 100, :test/updated 100}. Since there's a correspondence between relations and functions, it's possible to talk about injective/bijective/multivalued, but I don't think it's helpful in this case. It's better to think about our use cases:

if we ever want to symbolicate, entid_map gives the current association from entid to ident;
if we ever want to concretize, ident_map gives the current and historical associations from ident to entid.

This is what Datomic does, and I think it's the right call. It means that when the :db/ident associated to an entid changes, existing consumers can still refer to the entid with an outdated historical ident. And you can always re-purpose an ident if you really want to, breaking existing consumers that haven't updated.

Is that more clear?

rnewman · 2017-03-15T16:40:36Z

Is that more clear?

I think so. I think I have two points of friction:

I don't really get the names: ident and entid are each end of the same bidirectional association, and I'd expect to be able to work with current or historical versions of each — that is, if I've retracted [123 :db/ident :foo/bar], I want to be able to have schema.ident(123) == None.
I'm worried about the round-trip behavior. Depending on which maps are used where, you could end up with an input keyword being rewritten to be more modern, or worse — imagine a query like [:find ?x :where [?x _ :foo/bar]], which could incorrectly match against an entid that no longer is identified by that keyword. You'll get the wrong results!

ncalexan · 2017-03-16T23:07:59Z

@rnewman sorry about the force-push. This gets us to the following place:

no historical idents (so entid_map and ident_map are inverse to each other)
"idents" and "schema" materialized views are as they were in Datomish
no :db.{install,alter}/partition at all (see commit message for motivation)
[:db/retract e :db/ident ...] supported
[:db/retract :attribute :db/* ...] of schema characteristics not yet supported
[:db/retractEntity :attribute] not yet supported

I'll file follow-up for the last two if I don't do them as part of the next review cycle. Sorry for all this churn!

rnewman · 2017-03-17T19:12:21Z

 }

+#[derive(Clone,Debug,Eq,Hash,Ord,PartialOrd,PartialEq)]
+pub enum Unique {


Can we call this AttributeUniqueness or something more specific? This is in core.

Or a submodule for scoping? attribute::Unique::Value?

Thanks for correcting me on this; I should have known better. I've pushed it into an attribute submodule.

rnewman · 2017-03-17T19:17:37Z

-    if next_schema.is_some() {
-        // TODO Use custom ErrorKind https://github.com/brson/error-chain/issues/117
-        bail!(ErrorKind::NotYetImplemented(format!("Initial bootstrap transaction did not produce expected bootstrap schema")));
+    let bootstrap_schema_for_mutation = Schema::default(); // The bootstrap transaction will populate this schema.


rnewman · 2017-03-17T19:21:01Z

+fn read_schema_map(conn: &rusqlite::Connection) -> Result<SchemaMap> {
+    let entid_triples = read_materialized_view(conn, "schema")?;
+    let mut schema_map = SchemaMap::default();
+    metadata::update_schema_map_from_entid_triples(&mut schema_map, entid_triples)?;


Can you push this into a method on SchemaMap? SchemaMap::with_entid_triples?

I could, but I want the mutation form so it's &mut self in general. There's a symmetry in having the two updating functions close together (one operating on SchemaMap, the other on Schema) so I'm going to keep them in metadata.rs until we have more changes to make.

rnewman · 2017-03-17T19:23:10Z

+                     &[])?;
+        // Take the last, as determined by tx, added [e :db/ident v _ true].  This will pick up
+        // :db/idents that have been retracted and/or re-purposed.
+        conn.execute(format!("INSERT INTO idents SELECT e, a, v, value_type_tag FROM datoms WHERE a IN {}", entids::IDENTS_SET.as_str()).as_str(),


Careful: I suspect that this is implemented as a table walk with a filter, at least for the case of non-unit sets, which is startlingly expensive. You probably want to do this differently in the short or medium term.

Oh, and filtering on value_type_tag to keyword is something you might consider.

I expected this to be efficient because there's an index on e a v value_type_tag. Do you think it would be faster to have a bunch of WHERE a = ? OR a = ? clauses? A little playing with SQLite suggests the query plans are the same:

sqlite> EXPLAIN QUERY PLAN INSERT INTO idents SELECT e, a FROM datoms WHERE a = 0 OR a = 1; 0|0|0|SEARCH TABLE datoms USING COVERING INDEX idx_datoms_aevt (a=?) 0|0|0|EXECUTE LIST SUBQUERY 1 sqlite> EXPLAIN QUERY PLAN INSERT INTO idents SELECT e, a FROM datoms WHERE a IN (0, 1); 0|0|0|SEARCH TABLE datoms USING COVERING INDEX idx_datoms_aevt (a=?) 0|0|0|EXECUTE LIST SUBQUERY 1

Feel free to file follow-up if you're not convinced.

Query plan trumps vague suspicions!

rnewman · 2017-03-17T19:23:26Z

+    // TODO: use concat! to avoid creating String instances.
+    if !metadata_report.idents_altered.is_empty() {
+        // Idents is the materialized view of the [entid :db/ident ident _ added] slice of
+        // transactions.  This keeps outdated idents around, just like Datomic does.


I don't think this comment is still accurate.

rnewman · 2017-03-17T19:30:51Z

+    let mut insert_stmt = conn.prepare(format!("INSERT INTO schema SELECT e, a, v, value_type_tag FROM datoms WHERE e = ? AND a IN {}", entids::SCHEMA_SET.as_str()).as_str())?;
+    let mut index_stmt = conn.prepare("UPDATE datoms SET index_avet = ? WHERE a = ?")?;
+    let mut unique_value_stmt = conn.prepare("UPDATE datoms SET unique_value = ? WHERE a = ?")?;
+    let mut cardinality_stmt = conn.prepare("SELECT e FROM datoms WHERE a = ? GROUP BY e HAVING COUNT(*) > 1 LIMIT 1")?;


I might rephrase this to something potentially cheaper:

SELECT EXISTS ( SELECT 1 FROM datoms AS one, datoms AS two WHERE one.a = ?1 AND two.a = ?1 AND one.e = two.e AND one.v <> two.v ) AS dupes;

Sure, thanks for the suggestion.

rnewman · 2017-03-17T19:32:52Z

+    };
+
+    /// Attributes that are "schema related".  These might change the "schema" materialized view.
+    pub static ref SCHEMA_SET: String = {


Yikes. Call out that these are SQL lists, not Rust sets!

s/_SET/_SQL_LIST/.

This is consistent with the capitalization (which is "valueType") and the other identifier.

Datomic (and eventually Mentat) don't allow to retract :db/ident in this way, so this runs afoul of future work to support mutating metadata.

This looks ahead to a time when we could support arbitrary user-defined materialized views. For now, the "idents" materialized view is those datoms of the form [e :db/ident :namespaced/keyword] and the "schema" materialized view is those datoms of the form [e a v] where a is in a particular set of attributes that will become clear in the following commits. This change is not backwards compatible, so I'm removing the open current (really, v2) test. It'll be re-instated when we get to mozilla#194.

In order to support historical idents, we need to distinguish the "current" map from entid -> ident from the "complete historical" map ident -> entid. This is what Datomic does; in Datomic, an ident is never retracted (although it can be replaced). This approach is an important part of allowing multiple consumers to share a schema fragment as it migrates forward. This fixes a limitation of the Clojure implementation, which did not handle historical idents across knowledge base close and re-open. The "entids" materialized view is naturally a slice of the "datoms" table. The "idents" materialized view is a slice of the "transactions" table. I hope that representing in this way, and casting the problem in this light, might generalize to future materialized views.

This is just to keep track of the expected changes during bootstrapping. I want bootstrap metadata mutations to flow through the same code path as metadata mutations during regular transactions; by differentiating the schema used for interpretation from the schema that will be updated I expect to be able to apply bootstrap metadata mutations to an empty schema and have things like materialized views created (using the regular code paths). This commit has been re-ordered for conceptual clarity, but it won't compile because it references the metadata module. It's possible to make it compile -- the functionality is there in the schema module -- but it's not worth the rebasing effort until after review (and possibly not even then, since we'll squash down to a single commit to land).

I haven't taken your review comment about consuming AttributeBuilder during each fluent function. If you read my response and still want this, I'm happy to do it in review.

This "loops" the committed datoms out of the SQL store and back through the metadata (schema, but in future also partition map) processor. The metadata processor updates the schema and produces a report of what changed; that report is then used to update the SQL store. That update includes: - the materialized views ("entids", "idents", and "schema"); - if needed, a subset of the datoms themselves (as flags change). I've left a TODO for handling attribute retraction in the cases that it makes sense. I expect that to be straight-forward.

Also adds a little more commentary and a simple test.

…from idents." This reverts commit 23a91df. Following our discussion, this removes the "entids" materialized view. The next commit will remove historical idents from the "idents" materialized view.

…iews.

This is not necessary, but it was suggested that we might be paying an overhead creating Err instances while using error_chain. That seems not to be the case, but this change shows that we don't actually use any of the Result helper methods, so there's no reason to overload Result. This change might avoid some future confusion, so I'm going to land it anyway. Signed-off-by: Nick Alexander <nalexander@mozilla.com>

These tests fail due to a Datomic limitation, namely that the marker flag :db.alter/attribute can only be asserted once for an attribute! That is, [:db.part/db :db.alter/attribute :attribute] will only be transacted at most once. Since older versions of Datomic required the :db.alter/attribute flag, I can only imagine they either never wrote :db.alter/attribute to the store, or they handled it specially. I'll need to remove the marker flag system from Mentat in order to address this fundamental limitation.

I was finding it very difficult to track unwrapping errors while making changes, due to an underlying Mac OS X symbolication issue that makes running tests with RUST_BACKTRACE=1 so slow that they all time out.

I had this all working... except we will never see a repeated `[:db.part/db :db.alter/attribute :attribute]` assertion in the store! That means my approach would let you alter an attribute at most one time. It's not worth hacking around this; it's better to just stop expecting (and recognizing) the marker flags. (We have all the data to distinguish the various cases that we need without the marker flags.) This brings Mentat in line with the thrust of newer Datomic versions, but isn't compatible with Datomic, because (if I understand correctly) Datomic automatically adds :db.{install,alter}/attribute assertions to transactions. I haven't purged the corresponding :db/ident and schema fragments just yet: - we might want them back - we might want them in order to upgrade v1 and v2 databases to the new on-disk layout we're fleshing out (v3?).

This patch avoids a potential bug with the "schema" materialized view. If :db/unique :db.unique/value implies :db/index true, then what happens when you _retract_ :db.unique/value? I think Datomic defines this in some way, but I really want the "schema" materialized view to be a slice of "datoms" and not have these sort of ambiguities and persistent effects. Therefore, to ensure that we don't retract a schema characteristic and accidentally change more than we intended to, this patch stops having any schema characteristic imply any other schema characteristic(s). To achieve that, I added an Option<Unique::{Value,Identity}> type to Attribute; this helps with this patch, and also looks ahead to when we allow to retract :db/unique attributes.

The tests use strings, so they hide the chained errors which do in fact provide more detail.

This might be faster in practice.

jaredhirsch added the in progress label Mar 10, 2017

rnewman approved these changes Mar 11, 2017

View reviewed changes

ncalexan force-pushed the idents branch from dc6ee2d to f6360a6 Compare March 15, 2017 04:13

rnewman approved these changes Mar 15, 2017

View reviewed changes

ncalexan force-pushed the idents branch from f6360a6 to 15cdae5 Compare March 16, 2017 23:04

rnewman approved these changes Mar 17, 2017

View reviewed changes

ncalexan mentioned this pull request Mar 17, 2017

Support transacting :db/fulltext true attributes. Fixes #189. #375

Merged

ncalexan force-pushed the idents branch from 15cdae5 to d26af14 Compare March 20, 2017 20:11

ncalexan added 17 commits March 20, 2017 13:12

Pre: s/VALUETYPE/VALUE_TYPE/.

77266c3

This is consistent with the capitalization (which is "valueType") and the other identifier.

Pre: Don't retract :db/ident in test.

fed8dfd

Datomic (and eventually Mentat) don't allow to retract :db/ident in this way, so this runs afoul of future work to support mutating metadata.

Pre: Remove some single quotes from error output.

c0d3ee7

Pre: Map TypedValue::Ref to TypedValue::Keyword in debug output.

c708ae7

Pre: Add DiffSet.

057c45b

Part 4: Collect mutations to a Schema.

91ad2d8

I haven't taken your review comment about consuming AttributeBuilder during each fluent function. If you read my response and still want this, I'm happy to do it in review.

Review comment: Rename DiffSet to AddRetractAlterSet.

36633c1

Also adds a little more commentary and a simple test.

Review comment: Use ToIdent trait.

8483fd9

Review comment: partially revert "Part 2: Maintain entids separately …

6c34d83

…from idents." This reverts commit 23a91df. Following our discussion, this removes the "entids" materialized view. The next commit will remove historical idents from the "idents" materialized view.

Review comment: Don't preserve historical idents.

6172581

Review comment: More prepared statements when updating materialized v…

114d205

…iews.

ncalexan added 10 commits March 20, 2017 13:12

Post: Remove some more single quotes from error output.

add8144

Post: Add assert_transact! macro to unwrap safely.

b93e059

I was finding it very difficult to track unwrapping errors while making changes, due to an underlying Mac OS X symbolication issue that makes running tests with RUST_BACKTRACE=1 so slow that they all time out.

Post: Allow to retract :db/ident.

edac729

Post: Include more details about invalid schema changes.

2b39f02

The tests use strings, so they hide the chained errors which do in fact provide more detail.

Review comment: Fix outdated comment.

baedbb8

Review comment: s/_SET/_SQL_LIST/.

99c7217

Review comment: Use a sub-select for checking cardinality.

0ff0257

This might be faster in practice.

Review comment: Put attribute::Unique into its own namespace.

704b0a7

ncalexan force-pushed the idents branch from d26af14 to 704b0a7 Compare March 20, 2017 20:12

ncalexan merged commit 15b4195 into mozilla:rust Mar 20, 2017

jaredhirsch removed the in progress label Mar 20, 2017

ncalexan mentioned this pull request Mar 20, 2017

[tx] Handling retracting schema characteristics, like :db/cardinality and :db/unique. #379

Closed

Conversation

ncalexan commented Mar 10, 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!

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!

ncalexan commented Mar 15, 2017

Uh oh!

rnewman 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!

Choose a reason for hiding this comment

Uh oh!

rnewman commented Mar 15, 2017

Uh oh!

ncalexan commented Mar 15, 2017

Uh oh!

rnewman commented Mar 15, 2017

Uh oh!

ncalexan commented Mar 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!

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!