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

Proposal for range selectors #184

Merged
merged 3 commits into from
Mar 6, 2022
Merged

Proposal for range selectors #184

merged 3 commits into from
Mar 6, 2022

Conversation

willscott
Copy link
Member

This proposes a mechanism for selecting only a subset of a byte or string node that is an ADL over a larger number of nodes after reification.

@willscott willscott requested review from warpfork and mvdan March 1, 2022 18:14
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

yeah, seems reasonable to me (I like the key renames too)

Copy link
Contributor

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

I like the general outline of this. The data seems still placeholder though? Does something run these fixtures yet? I'm guessing not, because the data looks like an outline and doesn't look like it would actually fly.

I think that if this was fully real and wired to a working test, I would probably expect to see the fixtures grow to...

  • have some real (if small) content -- maybe just "abc" and "def" in the next chunk, etc
  • the hashes would have to become real
  • the size numbers, I guess, should become real as well
  • the subset indices in the selector would presumably become a bit smaller if we're using short fixture data
  • the visit probably looks totally different -- I'd expect this to yield one bytes node, wouldn't I?
  • which blocks are loaded is a different matter; maybe we want to have a fixture hunk for that as well (but we'd want both this and the expected visit, because they're very dissimilar, and that is highly worth making visible).

@warpfork
Copy link
Contributor

warpfork commented Mar 5, 2022

Putting subset optional Slice as a field in Matcher -- rather than as its own whole exploration clause (which could contain a matcher inside it) -- gave me some pause for thought... but I guess it seems fine, and reasonable considering that it's meant for use on strings and bytes, which are considered nonrecursives.

@willscott
Copy link
Member Author

the fixtures are waiting for ipld/go-ipld-prime#376 to complete - we don't currently have a way to convert a dag from the unix-fs dag-pb standard format to the json format that fixtures expect.

That said, this spec level change won't really be able to be evaluated for a bit still because we don't have a spec level definition of what 'unixfs' as an ADL does.

@willscott
Copy link
Member Author

agree that the visit will have 1 node (and that the current setup is less relevant for the traversal behavior of interest here.)

@willscott
Copy link
Member Author

Updated with a generated case of partial-selection over a unixfs file.
Note that this specification remains not fully complete at this level since it relies on what is currently a golang-specific implementation of unixfs.

@warpfork
Copy link
Contributor

warpfork commented Mar 6, 2022

That caveat is applicable indeed... but also with real data in it, this kinda presses some buttons in my brain labelled "gorgeous" :D

@willscott willscott merged commit b7f906c into master Mar 6, 2022
@willscott willscott deleted the select/adl-slice branch March 6, 2022 19:17
willscott added a commit to ipld/go-ipld-prime that referenced this pull request Mar 7, 2022
Implement an optional `LargeBytesNode` for an `AsLargeBytes() (io.ReadSeeker, error)` method. This allows, per the [selector spec](ipld/ipld#184) the ability to efficiently traverse sub-ranges of ADLs.
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.

None yet

3 participants