-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IPLD Amend #1
IPLD Amend #1
Conversation
2f05d9b
to
1de60d4
Compare
This is superwild and very interesting!! As an outline it definitely seems plausible that this can work. So I'll say no more about that. What's most interesting here is promoting the "patch" operations and structures... ... from where I drafted them in the ... into the core I'm a little nervous about that, to say the least. 😅 The feature overlap is pretty high, so it makes sense in that regard. But it's not 100% -- for example, I can't imagine what use the "test" operation would be here (which we have in turn largely just because it's specified in the JSON Patch RFC, and I was rolling with compatibility with that in mind when writing that patch package). I'm also not sure if a path to patch op target makes sense; just a pathsegment for where to update the value within the current node (no deeper) should suffice? And for a third, "move" and "copy" might be a little strange -- from a single node's perspective, when we're working with the in-memory handles to other nodes already, we don't really need those ops, do we? More important is the change coupling and inertia we'd be stuck with. I can't really provide evidence that would be bad, but I don't know if I would want to take a bet that it would be fine, either. I wonder if something very like this, but declaring new, unrelated types in the data model package, would be slightly more ideal. It would be more code, and some of it alarmingly similar looking, but it would decouple things more, and I suspect it might end up more refined in the end. |
datamodel/node.go
Outdated
type NodePrototypeSupportingAmend interface { | ||
AmendingBuilder(base Node) NodeBuilder | ||
AmendingBuilder(base Node) NodeAmendingBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started to wonder if NodePrototype is the right place to attach this -- and suspect not.
(yes, I realize this is my own old draft interface, and you're shifting it to be slightly more correct in this patch. heh.)
Larger comment back in the issue over here: ipld#320 (comment)
That makes sense. I'll try to play around with some options here. I didn't want to branch out too drastically, but more decoupling definitely seems like the better path. |
This is very interesting, thanks for thinking about this, @warpfork. I can see how I'm coming to the realization of I'll think about and play around with this some more soon. |
556c381
to
554144e
Compare
traversal/amend/amender.go
Outdated
panic("misuse") | ||
} | ||
func (a *amenderNode) Length() int64 { | ||
return a.base.Length() + int64(len(a.addOps)) - int64(len(a.remOps)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this can be accurate as far as a plain node is concerned, since we don't know anything about overlap and conflicts between add and remove ops (e.g. add a field twice); but maybe it doesn't matter here. But what would be the purpose or reporting a Length()
that has this base+add-remove calculation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coincidentally, I was just refactoring this :) I'm thinking of keeping a single map of mods. That way, the "effective" length calculation as well as the conflict resolution between adds/removes are simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A non-nil
value in the mods map means that a node was added (or added/removed/re-added, and so on), while a value of nil
means that it was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The currently pushed code definitely has bugs that I'm fixing (please don't hesitate to point them out). Correcting it to be more intelligent w.r.t. types of nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rvagg, I've iterated a couple of times and pushed another update with all (current) fixtures passing for map-type nodes.
It has a similar base+add-remove type calculation as before (with some adjustments) so that Encode
sees an accurate "effective length" for recursive nodes.
This approach also allows me to iterate over the contents more intelligently in order to provide an accurate "effective view" (lens?) that incorporates all accumulated removals and additions.
If you look at this code anytime soon (no rush from my side), @rvagg, I'm working on a better strategy for list-type nodes. The current approach appears to be working reasonably well for map-type nodes but I need to use an underlying I also have a question about what to do when an entirely new hierarchy gets created with a path that has a mixture of
...does this operation:
...result in this node (my preference):
...or this node?
Basically, I'm trying to determine the "right" type for a parent I thought of using the "type" of the segment, but it doesn't seem safe to assume that an It also doesn't seem safe to use the underlying storage type of
|
Hello, we're thinking of merging the initial version of the Patch APIs in go-ipld-prime. Would you like to sync up before we do so? ipld#350 If we don't hear back we'll merge in a couple of weeks. |
Thanks for the note, @RangerMauve. It's cool from my side to merge ipld#350 whenever you're ready. I've intentionally kept my current implementation separate and parallel to Eric's implementation. Can chat about this on the next IPLD team sync, if needed. I should have my implementation in a testable/reviewable state in a week or so, and be ready to benchmark both implementations. Assuming, of course, that the fundamental approach makes sense to @rvagg and @warpfork. It's still a WIP but my next iteration should be closer to the final state I have in mind. |
@smrz2001 Sweet. Lets talk on the next call and I'll merge the PR after that. 😁 |
@smrz2001 there's some overlapping discussion on PathSegment type stuff over @ ipld/ipld#195 which is also looking at patch-like semantics but for URLs. I think I'm mostly of the opinion that if you use paths, then you should just put up with what paths give you - which is basically something like: integers become indexes of lists and everything else is map keys. If you want to break out of this then you have to use an alternative mechanism. Which I think might be a case for either planning for a future extension to add more sophisticated specifiers than just plain paths, or building in an escaping mechanism to path semantics (although it could be argued that we're far too late for changing path semantics at all). |
Thanks, @rvagg, this is very helpful! |
@rvagg @RangerMauve, I've cleaned up the code, added comments, got all the tests to pass, and marked the PR ready for review. I'll continue to tinker with the fixtures to cover more corner cases as well as add more comments. I also feel like I could have used Golang more idiomatically but I'm not a Golang expert by any means so would also appreciate suggestions there. I haven't implemented link traversal but have an idea of how and where it should be implemented. What would you recommend for next steps? Benchmarking? |
c1d184f
to
c01bde4
Compare
c01bde4
to
6c654fa
Compare
Sorry for the force pushes... I detest git submodules - I just took a clean branch from |
6c654fa
to
3db6926
Compare
@rvagg @RangerMauve, just pushed some crude benchmarking tests comparing the two implementations (calling them I see some patterns already based on the size of the base My choice of how to implement the test was purely based on how quick/easy it was to write lol, but I might have unintentionally introduced some bias and will refine the tests some more. I should probably also add a check at the end of each test to make sure the serialized results are correct/consistent. Other than "remove" for lists, I was to able to run other operations ("add" and "replace") with both implementations using both recursive types. Here are some results for a N-sized map/list -> 1% / 10% / 100% update operations -> I only did 10 runs to start with to save some time iterating on the longer tests.
|
Would it make sense to replace the current patch implementation with this instead? e.g. expose an API to apply a patch set using the schema, but to have the underlying stuff using these new APIs / expose the raw APIs for when folks don't want to use patch sets? |
Hey @RangerMauve, I think it'd probably be good to wait till @warpfork is back and also has a chance to review the code and some of the design choices. There are also a few items that are not yet implemented in this version of the code that are available in I can take a stab at implementing those as well before considering the PR finalized so that we don't regress what's currently available. |
Closing in favor of official IPLD PR. |
This PR implements a possible solution for incrementally modifying IPLD nodes.
Node
updates but needs further optimization.Node
"lens" for copy-on-write duringEncode
.TestSpecFixtures/removing-map-entry
.TestSpecFixtures/replacing-map-entry
.TestSpecFixtures/copy
.TestSpecFixtures/move
.Amender
interface.TestSpecFixtures/inserting-into-a-list
.TestSpecFixtures/test-and-conditional-modify
.patch
implementation