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

Add ordering option to getPath #19

Merged
merged 4 commits into from
May 18, 2023

Conversation

hannahhoward
Copy link
Member

Goals

This adds support for depth first traversals to Dagula. It's designed to be in conformance with ipfs/specs#412.

(will update if parameter names change)

Implementation

  • adds order parameter to options struct for getPath and get
  • renames breadFirstSearch to blockLinks (it isn't really "bread first search" so much as "all the links in this block").
  • handles ordering change inside of get, using recursion for depth-first-search
  • adds tests for both orders.

Comment on lines -43 to +44
const search = options.search || breadthFirstSearch()
const search = options.search || blockLinks()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, the half-formed idea here was that the search fn could be used to let a caller pass in a stateful depth first search equivalent to the default "just return all the links in the current block", but i didn't try and implement it to try out that idea.

...all i really needed at the time was a way to pass in a rule for filtering out some links so i could implement car-scope: file dag-scope: entity for a hamt and got carried away.

search could go away in favour of linkFilter: ([name, cid]) => boolean or similar, and we could just have this get fn always do depth first. There was no strong reason for the previous default, and we can return depth-first for rnd as well.

Copy link
Contributor

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

news just in! the transform function used here doesn't guarantee order of results!
https://github.com/web3-storage/dagula/blob/ceca20a7da7f976777d4e77297eb6aada45f67d9/index.js#L52

Order is determined by when func resolves.
– https://github.com/bustle/streaming-iterables#transform

so for us to guarantee dfs that would need to switch out for parallelMap. I didn't check that when I named the default search impl breadth-first, so it was already misleading.

@hannahhoward
Copy link
Member Author

hannahhoward commented May 17, 2023

@olizilla used parallelMap for DFS. left transform in place otherwise -- I dunno if the speed optimization matters but I figured as little substantive change to the existing workflow the better :)

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

This LGTM but we should remove the search param (as mentioned here), switch to dfs by default and rename carScope to dagScope before release.

I'll send a follow up PR(s) to get those things done.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@alanshaw alanshaw merged commit ad25001 into storacha:main May 18, 2023
@alanshaw
Copy link
Member

@hannahhoward thank you for the contribution ❤️

alanshaw pushed a commit that referenced this pull request May 18, 2023
🤖 I have created a release *beep* *boop*
---


##
[7.0.0](v6.0.2...v7.0.0)
(2023-05-18)


### ⚠ BREAKING CHANGES

* remove search option
([#23](#23))
* rename dagScope to carScope
([#21](#21))

### Features

* add ordering option to getPath
([#19](#19))
([ad25001](ad25001))
* support yamux muxer
([#11](#11))
([24ef997](24ef997))


### Bug Fixes

* rename dagScope to carScope
([#21](#21))
([497cc90](497cc90))


### refactor

* remove search option
([#23](#23))
([9ede86f](9ede86f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants