Implement ZSink.collectAllN #4923
Conversation
| @@ -346,7 +346,7 @@ object ZSink { | |||
| * A sink that collects all of its inputs into chunks of maximum size `n`. | |||
| */ | |||
| def collectAllN[Err, In](n: Int): ZSink[Any, Err, In, Err, In, Chunk[In]] = | |||
| foldUntil(Chunk.empty: Chunk[In], n.toLong)(_ appended _) | |||
| foldUntil(Chunk.empty: Chunk[In], n.toLong)(_ :+ _) | |||
There was a problem hiding this comment.
Should we be using a ChunkBuilder here?
There was a problem hiding this comment.
Would be neat, but it's one extra step to materialize it. Should be safe otherwise.
There was a problem hiding this comment.
Ah a ChunkBuilder is a good point. We can also hint it with the size so should be significantly more efficient.
There was a problem hiding this comment.
Tackled in 0427c09.
Edit: scratch this, it's not good.
There was a problem hiding this comment.
Here's the updated version: 375647a
| @@ -342,6 +342,14 @@ object ZSink { | |||
| new ZSink(loop(Chunk.empty)) | |||
| } | |||
|
|
|||
| /** | |||
| * A sink that collects all of its inputs into chunks of maximum size `n`. | |||
There was a problem hiding this comment.
Can you also add a note that every chunk is preallocated and must fit in memory? Or @adamgfraser should we use the same approach you arrived at with regards to the size hint? (Which I don't remember what it was 😅)
There was a problem hiding this comment.
I think I've come to the point of view that we should just hint the specified size if an operator allows specifying a particular chunk size. I think we have to do that for maximum efficiency and it is just user error if you specify a chunk size like Int.MaxValue.
I was a little hesitant on this because of an issue we had with someone blowing themselves up using queue.takeUpTo(Int.MaxValue) but I think that is different in that there the n is how many elements to take and chunking is an implementation detail, whereas here the whole point of the parameter is to specify the size of the chunk to take.
e810e5a to
b8f6ea9
Compare
| @@ -20,6 +20,14 @@ object ZSinkSpec extends ZIOBaseSpec { | |||
| def spec: ZSpec[Environment, Failure] = { | |||
| suite("ZSinkSpec")( | |||
| suite("Constructors")( | |||
| suite("collectAllN")( | |||
There was a problem hiding this comment.
Same re tests from ZTransducerSpec :-)
There was a problem hiding this comment.
Done in 239c554. As discussed over chat, we have to leave with trailing chunks, therefore the nonapplicable tests are dropped.
b8f6ea9 to
239c554
Compare
Part of #4886