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

EXPERIMENTAL: map/list interfaces #4

Closed
wants to merge 1 commit into from

Conversation

smrz2001
Copy link
Owner

@smrz2001 smrz2001 commented Aug 1, 2022

Very experimental changes for (hopefully) more usable map and list interfaces for IPLD nodes, based on amend.

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.

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 17:22
@RangerMauve
Copy link

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

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.

@smrz2001
Copy link
Owner Author

smrz2001 commented Aug 1, 2022

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!

@RangerMauve
Copy link

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

@smrz2001
Copy link
Owner Author

smrz2001 commented Aug 1, 2022

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
Copy link
Owner Author

smrz2001 commented Aug 1, 2022

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.

@smrz2001 smrz2001 changed the base branch from patch-feature-with-amend to master August 1, 2022 21:59
@smrz2001 smrz2001 changed the base branch from master to patch-feature-with-amend August 1, 2022 22:03
@smrz2001
Copy link
Owner Author

smrz2001 commented Aug 1, 2022

Closing in favor of PR against main repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants