fix rechunkRandomly with large ending chunk #3277
Conversation
| @@ -1042,4 +1042,13 @@ class StreamSuite extends Fs2Suite { | |||
| } | |||
| } | |||
|
|
|||
| test("rechunkRandomlyWithSeed should correclty rechunk big chunks at the end of a stream") { | |||
There was a problem hiding this comment.
Looks like we keep those tests here.
Co-authored-by: Arman Bilge <armanbilge@gmail.com>
| } | ||
|
|
||
| underlying.uncons.flatMap { | ||
| case None => Pull.done |
There was a problem hiding this comment.
Idea, since the new implementation is slightly more complex anyway, maybe we could avoid creation of the random object (val random = new scala.util.Random(seed)) in this empty case, since it's then not used? Or perhaps it's not worth it, not sure. 🙂
There was a problem hiding this comment.
We could, but I'm not sure if it's worth it. It'd only have any effect if the random rechunking was called repeatedly on empty streams, which seems like an extraordinarily niche scenario.
Anyway, I'm happy to do it if told otherwise, but I'll save the CI electricity for now
| .chunks | ||
| .compile | ||
| .toList | ||
| assert(chunks.forall(_.size < 500)) |
There was a problem hiding this comment.
Just a small comment, if I understand the documentation and the implementation of the rechunkRandomlyWithSeed correctly, then the minimum and maximum size factors are inclusive, so maybe the assertion should actually be:
| assert(chunks.forall(_.size < 500)) | |
| assert(chunks.forall(_.size <= 500)) |
But I could be wrong about that. 🙂
There was a problem hiding this comment.
You're right, it should be inclusive
the contract correctly
|
CI seems to have broken on some unrelated part of code |
| go(acc ++ hd, size, tl) | ||
| s.uncons.flatMap { | ||
| case None => Pull.output(acc) | ||
| case Some((hd, tl)) => |
There was a problem hiding this comment.
Nit: while reading the newest iteration of the code I had to do a double take, because there is now shading here (hd and tl used in the go function hide the variables with the same names from the higher scope of underlying.uncons.flatMap). Maybe it would be nice to avoid this for the sake of clarity. 🙂
|
#3195 also seems to have progressed after this PR. You no longer get 1 very large |
Fixes #3276
Not sure if I put the test in the right place, let me know if you'd like to move it!