-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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/datamodel selector retrieval #6393
Conversation
Review note: this is the first user-facing selector interface that I know of. Also selector execution is not something we have a lot of prior art for. Request for @warpfork @mvdan @willscott and @hannahhoward to very carefully examine these parts:
|
This comment has been minimized.
This comment has been minimized.
node/impl/client/client.go
Outdated
ssb := builder.NewSelectorSpecBuilder(basicnode.Prototype.Any) | ||
selspec := ssb.ExploreRecursive( | ||
selector.RecursionLimitNone(), | ||
ssb.ExploreAll(ssb.ExploreRecursiveEdge()), |
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.
do we want a matcher here as well?
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 opted for
lotus/node/impl/client/client.go
Lines 738 to 739 in efeaea7
// FIXME - this is a direct copy from https://github.com/filecoin-project/go-fil-markets/blob/v1.4.0/shared/selectors.go#L11-L16 | |
// Unable to use it because we need the SelectorSpec, and markets exposes just a reified node |
Diverging didn't make sense... @hannahhoward should comment here: either we export the spec, or we adjust both
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The deal data selector is now available so if folks actually want to retrieve data from Filecoin they'll soon be able to use this (🔜 currently needs custom lotus client filecoin-project/lotus#6393). resolves #192
The deal data selector is now available so if folks actually want to retrieve data from Filecoin they'll soon be able to use this (🔜 currently needs custom lotus client filecoin-project/lotus#6393). resolves #192
This comment has been minimized.
This comment has been minimized.
70114b6
to
3598eff
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7ae0512
to
ab1377f
Compare
08ee9df
to
988d14b
Compare
Syntaxt of selection is located at https://pkg.go.dev/github.com/ipld/go-ipld-selector-text-lite#SelectorSpecFromPath Example use, assuming that: - The root of the deal is a plain dag-pb unixfs directory - The directory is not sharded - The user wants to retrieve the first entry in that directory lotus client retrieve --miner f0XXXXX --datamodel-path-selector 'Links/0/Hash' bafyROOTCID ~/output For a much more elaborate example see the top of ./itests/deals_partial_retrieval_test.go
06f5253
to
af0d9b6
Compare
Codecov Report
@@ Coverage Diff @@
## master #6393 +/- ##
==========================================
+ Coverage 39.14% 39.57% +0.43%
==========================================
Files 614 617 +3
Lines 64997 65373 +376
==========================================
+ Hits 25440 25871 +431
+ Misses 35150 34996 -154
- Partials 4407 4506 +99
Continue to review full report at Codecov.
|
This PR has been updated once again to track latest master. In fact it had to be merged on top of (and to thus include) #7441, because There are now negative-tests addressing most of the comments above, everything is green, in fact coverage went up. Also an identical CLI flag with the same functionality got merged into the |
Which interface, out of curiosity? Nothing came to mind between v0.12.0 and v0.12.3, and I just skimmed the list of commits and didn't spot anything obvious. |
|
Ahh, sorry, I suppose that was me. Trying to move away from exposing bare |
( This PR depends on and includes #6375 )✅Introduce a new
RetrievalOrder
-struct field and a CLI option that takes a string representation as understood by https://pkg.go.dev/github.com/ipld/go-ipld-selector-text-lite#SelectorSpecFromPath . Allows for partial retrieval of any sub-DAG of a deal provided the user knows the exact low-level shape of the deal contents.As an example with this patch one can retrieve the first entry of a UnixFS directory by executing:
lotus client retrieve --miner f0XXXXX --datamodel-path-selector 'Links/0/Hash' bafyROOTCID ~/output
See top of itests/deals_partial_retrieval_test.go for a more elaborate example.