Skip to content
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

feat: map/list interfaces #451

Closed
wants to merge 13 commits into from
Closed

Conversation

smrz2001
Copy link
Contributor

@smrz2001 smrz2001 commented Aug 1, 2022

This depends on #445.

Experimental changes for more usable map and list interfaces for IPLD nodes, based on amend.

This is an attempt to:

  • Establish a more user-friendly interface for recursive types. A big pain point for me (and others) has been the current (rather inconvenient) way to use maps or lists (builders, assemblers, etc.)
  • Allow modification transparently and with all the optimizations amend brings.

Once a reasonable interface is established, it can be extended to other recursive types/ADLs like go-ipld-adl-hamt, that are also quite inconvenient (or impossible) to use like normal maps/lists.

I wanted to get some early feedback so only the map interface is implemented. The tests in map_test.go demonstrate some ways in which the interface can be used.

Supersedes this PR.

cc @RangerMauve @rvagg @aschmahmann @BigLep @warpfork

type Map interface {
	Put(key string, value interface{}) bool
	Get(key string) (value Node, found bool)
	Remove(key string) bool
	Keys() []string

	Container
}
type List interface {
	Get(idx int64) (Node, bool)
	Remove(idx int64)
	Append(values ...interface{})
	Insert(idx int64, values ...interface{})
	Set(idx int64, value interface{})

	Container
}
type Container interface {
	Empty() bool
	Length() int64
	Clear()
	Values() []Node
}

@smrz2001 smrz2001 marked this pull request as draft August 1, 2022 22:56
@smrz2001
Copy link
Contributor Author

smrz2001 commented Aug 1, 2022

Copying over some of your comments and my responses, @RangerMauve, so that the full discussion is in one place.

smrz2001#4 (comment)

Could you elaborate a bit more on the motivation for introducing these types? Particularly what in the existing codebase is lacking that this addresses.

Yes, sorry, I should have added more context. This is an attempt to:

  • Establish a more user-friendly interface for recursive types. A big pain point for me (and others) has been the current, inconvenient way to use maps or lists (builders, assemblers, etc.)
  • Allow modification transparently and with all the optimizations amend brings.

Once a reasonable interface is established, it can be extended to other map types/ADLs like HAMT, that are also quite inconvenient to use like normal maps.

Also, regarding methods that return arrays, would it be possible to return iterators instead? There are ADLs where a Map's keys might potentially be too large to store in memory, similarly with Lists. I think we should have streaming in consideration from the get go so that we don't run into issues later.

Yes, makes sense! I wasn't too thrilled about them and was considering just removing them but returning iterators is much better.

Thanks, @RangerMauve!

smrz2001#4 (comment)

Does golang have a concept of async iterators? I think Adin brought it up on the IPFS discord. 😁

Good question 😊 I've also been thinking about it since I read Adin's comments. I think it's possible to provide thread-safe, concurrent iteration in the current model, with some tweaks.

Can chat a bit about this on the call too, if we have time.

smrz2001#4 (comment)

I also wanted to pass another thought by you, @RangerMauve.

I can't get the idea out of my head that what amend really is, is an updatable ADL. It takes a source Node, allows the accumulation of updates, then "presents" a new Node to consumers.

I could easily turn Amender.Build into Reify and add Substrate to expose the source Node.

@rvagg rvagg self-requested a review August 2, 2022 22:09
@RangerMauve RangerMauve mentioned this pull request Aug 3, 2022
@BigLep BigLep added the P2 Medium: Good to have, but can wait until someone steps up label Oct 4, 2022
@smrz2001
Copy link
Contributor Author

smrz2001 commented May 8, 2023

Closing this PR for now until (if at all) it makes sense to pick back up. I'd like to take this over the finish line but don't want to add work unnecessarily.

@smrz2001 smrz2001 closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Medium: Good to have, but can wait until someone steps up
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

2 participants