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 Iterable to support streaming #38

Merged
merged 11 commits into from Jan 14, 2022
Merged

Conversation

huyuncong
Copy link
Member

Description

Add streaming utilities as Iterable into the std library.
The struct supports future developments for streaming.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

src/iterable/mod.rs Outdated Show resolved Hide resolved
src/iterable/mod.rs Outdated Show resolved Hide resolved
src/iterable/mod.rs Outdated Show resolved Hide resolved
src/iterable/mod.rs Outdated Show resolved Hide resolved
mmaker and others added 5 commits January 7, 2022 17:57
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
This code was contributed as part of Gemini <https://github.com/mmaker/gemini>.
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
src/iterable/mod.rs Outdated Show resolved Hide resolved
src/iterable/mod.rs Outdated Show resolved Hide resolved
src/iterable/mod.rs Outdated Show resolved Hide resolved
/// The type of the iterator being generated.
type Iter: Iterator<Item = Self::Item>;

/// Returns the iterator associated to the current instance.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns the iterator associated to the current instance.
/// Returns the iterator associated to the current instance.

src/iterable/mod.rs Outdated Show resolved Hide resolved
src/iterable/mod.rs Outdated Show resolved Hide resolved
///
/// An Iterable is pretty much like an [`IntoIterator`] that can be copied over
/// and over, and has an hint of the length.
/// In other words, these two functions are pretty much equivalent.
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like the entire Iterable trait is not necessary, and it is the duty for Gemini to not use Iterable but instead to use the standard library one.

What is the main difference that Iterable provides but not by IntoIterator?

Copy link
Member

@mmaker mmaker Jan 11, 2022

Choose a reason for hiding this comment

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

Fair point: the main difference is that we need to iter over the same stream multiple times, by design.
This means that already we require IntoIterator + Copy or IntoIterator + Clone if we want to go down that avenue.

From there, it's pretty much the same, except that now to get the length of a stream it becomes a bit more verbose, and the length function might disagree with the actual length (this is needed for instance when streaming a matrix).
Here's a bit of an overview of how the code might differ: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c7d9d2fca7d1d308601bd5b32d770409

To be fair, on the one hand, the downside is that Iterable::iter will have to copy all relevant data from the initial structure (until https://github.com/rust-lang/rfcs/blob/master/text/1598-generic_associated_types.md hits the streets). On the other hand, for the long-term vision we have a trait that can be extended with other functions on the top of normal iterables, and that we skip having to type things like vec.into_iter().len().

mmaker and others added 5 commits January 11, 2022 19:30
Co-authored-by: Weikeng Chen <w.k@berkeley.edu>
Co-authored-by: Weikeng Chen <w.k@berkeley.edu>
Co-authored-by: Weikeng Chen <w.k@berkeley.edu>
Co-authored-by: Weikeng Chen <w.k@berkeley.edu>
Co-authored-by: Weikeng Chen <w.k@berkeley.edu>
@Pratyush Pratyush merged commit 452f5d5 into arkworks-rs:master Jan 14, 2022
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

4 participants