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(bindnode): support listpairs struct representation #514

Merged
merged 6 commits into from
Jun 5, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 20, 2023

I want order-preserving structs without a pre-defined strict order (tuple), so this implements listpairs which is in the schema spec but has sparse support through go-ipld-prime.

type Blop struct {
  foo optional Int
  bar optional String
  baz optional String
} representation listpairs
type Blop struct {
  Foo *int
  Bar  *string
  Baz *string
}
[ "foo", 100, "baz", "bing" ]

(edit: actually should be this:)

[ [ "foo", 100 ], [ "baz", "bing" ] ]

But also this should decode to the same struct (see out-of-order test):

[ "baz", "bing", "foo", 100 ]

(edit: actually should be this:)

[ [ "baz", "bing" ], [ "foo", 100 ] ]

@rvagg rvagg requested review from masih and willscott April 20, 2023 13:14
w.nextIndex++
if idx%2 == 0 {
asm := (*_structAssembler)(w).AssembleKey()
// ???
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to have a comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, embarrassing ... that's there cause I don't actually know why I would want to put the next line there; I intended to figure this out

@rvagg rvagg marked this pull request as draft April 20, 2023 13:17
@rvagg
Copy link
Member Author

rvagg commented Apr 20, 2023

marked as draft cause I just realised after writing out the example above that listpairs is actually [[k1,v1],[k2,v2]], so a list of k/v tuples; so I need to update the repr to look like that!

@rvagg rvagg marked this pull request as ready for review April 25, 2023 00:58
@rvagg
Copy link
Member Author

rvagg commented Apr 25, 2023

OK, fixed, this now handles [[k1,v1],[k2,v2]...]. It's required a bit more code because of the need for new intermediate assemblers for the inner lists that are strictly length of two, so we have _listpairsFieldAssemblerRepr and _listpairsFieldListAssemblerRepr to set that up properly.

I also extended the test a bit more so we're not just setting strings as values but also adding a recursive kind on one of the fields, so we end up doing a [["foo","0"],["bar","1"],["baz","2"],["qux",["3","4"]]] where the qux field is a list of strings.

@rvagg
Copy link
Member Author

rvagg commented Apr 26, 2023

Made the parser handle "listpairs" for structs in the DSL, including enabling the test fixture that's been waiting to be implemented: ipld/ipld#281

node/bindnode/repr.go Outdated Show resolved Hide resolved
Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

current impl works on scalar assemblies but borks when you try to assemble a
child that has a non-scalar; so nested listpairs in listpairs doesn't work
because we've not been passing representation assemblers properly
@rvagg rvagg force-pushed the rvagg/bindnode-listpairs branch from 95a91b0 to e3ab704 Compare May 13, 2023 08:51
@rvagg
Copy link
Member Author

rvagg commented May 13, 2023

good thing I held off until I had a chance to properly try and use this, it borked on complex nested values—trying to do a listpairs within a listpairs errored out; very tricky to get the "level" right with these builders, representation builders use the schema builders but always need to force them to drop back down to representation level when returning sub-builders to the user

hopefully fixed now, still trying to put it to use

@rvagg rvagg merged commit c951feb into master Jun 5, 2023
@rvagg rvagg deleted the rvagg/bindnode-listpairs branch June 5, 2023 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

2 participants