Repository navigation
mtca: calculate covering subtrees and store in DB. - #9153
Conversation
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
I think this TileHashReader-based approach is clear and obviously correct, and we shouldn't try to do a Frontier-based optimization yet. We can do that as an optional improvement later.
In addition to integrating review feedback, during our pair-coding session we noticed that we'd really like to rename the CheckpointSubtree stuff to reflect the renamed table which is simply "subtrees".
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
LGTM with nits. I think it would be fine to break out the GetCheckpointSubtree -> GetSubtree change into a separate PR, but I also don't think it's necessary. Happy to defer to other reviewers on that.
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Looks good. As an FYI: dbStoredSubtree winds up looking a lot like treedb.SubtreeModel. And note the comment on treedb.InsertSubtree():
Because borp's `Insert` modifies its argument to set the ID
when a field is marked as autoincrement (`.SetKeys(true, "ID")`), this method
also modifies its argument (in this case, `model`).
In other words, after calling InsertSubtree() we have a handy SubtreeModel object that carries the start and end we want, as well as the new ID. When I originally wrote this function, I was unsure as to whether it was better to return an ID, or simply rely on the mutate-in-place logic. Here it seems like perhaps the latter.
Alternatively, we could decide we actually don't like the way borp mutates things in place during Insert, and rely solely on the returned ID - which would be consistent with the earlier feedback I gave about mutation!
At any rate, I think this is good to merge and we can fiddle with it later if we see fit. :D
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
LGTM modulo one thing we discussed in the call yesterday
8d211fd
Fixes: #9038