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

Implement unchunks (inverse of chunks) #2506

Merged
merged 4 commits into from
Jul 27, 2021
Merged

Conversation

armanbilge
Copy link
Member

This seemed a strange omission to me, but maybe there's a good reason for it?

@mpilquist
Copy link
Member

There's already unchunk which (unfortunately) does something different and we never found a name that we were happy with.

@armanbilge armanbilge changed the title Implement chunks (inverse of unchunks) Implement unchunks (inverse of chunks) Jul 26, 2021
@djspiewak
Copy link
Member

The only concern I see is the fact that unchunk is a function which you almost never want to touch, while unchunks is a perfectly normal thing that you'll do all the time. :-P

@armanbilge
Copy link
Member Author

armanbilge commented Jul 26, 2021

The only concern I see is the fact that unchunk is a function which you almost never want to touch

If this is true, can we deprecate and rename unchunk? Would deprecation be enough warning when writing new code, is this really the method you want to use?

@mpilquist
Copy link
Member

I'd be fine with deprecating unchunk and giving its replacement a scarier name like destroyChunks or unitaryChunks or something?

@armanbilge
Copy link
Member Author

It's a very obtuse method actually, because the change is happening under-the-hood right? What are its use cases? I would guess maybe when you don't want eager evaluation, so you don't lose a whole chunk on error? Could the name reflect something about that?

@djspiewak
Copy link
Member

Yeah I've used O.G. unchunks like… once… The use-case is when you have a downstream flatMap which produces considerably more data on a per-element basis. If you don't unchunk in that case you can have memory issues.

@armanbilge
Copy link
Member Author

armanbilge commented Jul 26, 2021

Another idea: if the OG unchunk use-case is so rare, why don't we just suggest replacing with .chunkLimit(1).unchunks?

I think this makes it even clearer exactly what is going on.

@djspiewak
Copy link
Member

Another idea: if the OG unchunk use-case is so rare, why don't we just suggest replacing with .chunkLimit(1).unchunks?

This is superb.

@mpilquist
Copy link
Member

With current implementation of chunkLimit, that will be a bit different (compare expanded form of uncons1 with unconsLimit(1)) -- we should probably fix that

@armanbilge
Copy link
Member Author

@mpilquist excuse my density, what exactly is the difference?

@mpilquist
Copy link
Member

The special casing which avoids the cons call when chunk size is equal to 1

@armanbilge
Copy link
Member Author

I still don't see how that makes a difference, other than performance? Since cons will ignore an empty chunk.

@mpilquist
Copy link
Member

Only performance, though if full expansion is literally only the call to cons and doesn't actually change anything, then disregard

@armanbilge
Copy link
Member Author

The special case avoids the cons and the drop/splitAt. I can make this change?

@armanbilge
Copy link
Member Author

But btw, how important is performance here anyway? In @djspiewak's use-case if the stream is about to get slammed by flatMapping to big chunks does this really matter?

@@ -2588,6 +2588,7 @@ final class Stream[+F[_], +O] private[fs2] (private[fs2] val underlying: Pull[F,
* res0: List[Chunk[Int]] = List(Chunk(1), Chunk(2), Chunk(3), Chunk(4), Chunk(5), Chunk(6))
* }}}
*/
@deprecated("Use .chunkLimit(1).unchunks instead.", "3.0.7")
Copy link
Member

Choose a reason for hiding this comment

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

This should have introduced a bunch of deprecation warnings in the test suite. Can we address those too?

Copy link
Member Author

Choose a reason for hiding this comment

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

On it.

Copy link
Contributor

@Daenyth Daenyth left a comment

Choose a reason for hiding this comment

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

I've wanted this since Segment was still a thing

@mpilquist mpilquist merged commit 5d9845f into typelevel:main Jul 27, 2021
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.

4 participants