Rewrite `MultiProduct` #835

Philippe-Cholet · 2024-01-03T18:30:55Z

Fixes #337. Closes #603 since this is an alternative.

#834 legitimately asked that we work on the bugfix #603. But after rebasing it, I saw that I could finally fuse the iterator (and therefore have a working specialization test). I thought of using #603 as a base but I eventually chose to make my own changes, starting over. I however credited @JakobDegen where it made sense.

I made commits as atomic as I could.

So this fuses the iterator which is a breaking change, which won't affect most people.

phimuemue

Thank you @Philippe-Cholet .

We should do this.

jswrenn · 2024-01-03T22:01:47Z

Thanks for this! I'll give it a review tomorrow!

Philippe-Cholet · 2024-01-04T14:20:16Z

TL;DR The choice I made in db5d906 does not seem to affect performance.

I just tried cur: Option<I::Item>, in each MultiProductIter (as before in "master") instead of cur: Option<Vec<I::Item>>, in MultiProductInner (as in this PR), benchmarked both and got similar performances (both a bit slower for next than in "master" +8%, but with bugfix and fused).

EDIT: It's not as polished as this PR but this is the alternative I'm talking about.

jswrenn

This looks good to me, but needs some more documentation. For starters, could you add some more documentation to the type definitions?

src/adaptors/multi_product.rs

codecov-commenter · 2024-01-18T16:56:56Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (62d2b65) 93.43% compared to head (085990c) 93.69%.

Files	Patch %	Lines
src/adaptors/multi_product.rs	97.56%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   93.43%   93.69%   +0.25%     
==========================================
  Files          48       48              
  Lines        6778     6755      -23     
==========================================
- Hits         6333     6329       -4     
+ Misses        445      426      -19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Philippe-Cholet · 2024-01-18T16:57:08Z

I rebased on master and added 3 commits to comment "state and values" with code.

phimuemue

Thanks

By wrapping "inner" in an option, I'll be able to fuse the iterator. Keep the current value of each iterator in `cur`: none at the beginning, some later. Previously, each `MultiProductIter` remembered its own element. Now, we have a vector of them. That way, we can update `cur` in-place and clone it to generate the item, I think that's simpler (less unwrap) and maybe more efficient. But we have two different vectors instead of a single bigger vector.

Co-Authored-By: Jakob Degen <[email protected]>

An empty product is supposed to generate a single empty vector, test this. Co-Authored-By: Jakob Degen <[email protected]>

`count` is generally not a cheap operation so I avoid it when the result would be zero anyway.

Similar to `count` but with "size hint" operations.

While adding comments, I realized I could set the iterator as finished in the case of the nullary cartesian product. It adds a simple invariant. I thought of making a comment but a debug check is better.

Separate `Populated/NotYetPopulated` in `match` blocks. Indents change quite a bit.

Philippe-Cholet · 2024-01-22T13:10:09Z

I merely rebased (no conflict) to see if the new CI still accepts this.
Do we merge this (and #853) now or in the next release?

jswrenn · 2024-01-22T16:37:18Z

I'll cut a release with our non-breaking changes first, probably tomorrow.

Philippe-Cholet · 2024-01-30T07:59:38Z

Now that "0.12.1" is released, I renamed the associated milestone and created a new one.
I think it's time to merge this and approve/merge #853 as well, right?

Signed-off-by: Richard Chien <[email protected]>

Philippe-Cholet added bug breaking-change labels Jan 3, 2024

Philippe-Cholet added this to the next milestone Jan 3, 2024

phimuemue approved these changes Jan 3, 2024

View reviewed changes

jswrenn requested changes Jan 7, 2024

View reviewed changes

src/adaptors/multi_product.rs Outdated Show resolved Hide resolved

src/adaptors/multi_product.rs Outdated Show resolved Hide resolved

Philippe-Cholet requested a review from jswrenn January 7, 2024 15:23

jswrenn approved these changes Jan 8, 2024

View reviewed changes

src/adaptors/multi_product.rs Outdated Show resolved Hide resolved

phimuemue reviewed Jan 8, 2024

View reviewed changes

src/adaptors/multi_product.rs Outdated Show resolved Hide resolved

phimuemue reviewed Jan 8, 2024

View reviewed changes

src/adaptors/multi_product.rs Outdated Show resolved Hide resolved

jswrenn mentioned this pull request Jan 16, 2024

ConsTuples: remove DoubleEndedIterator implementation #853

Merged

Philippe-Cholet force-pushed the rewrite-multiproduct branch from 05c2edf to d0d8d4f Compare January 18, 2024 16:55

jswrenn approved these changes Jan 18, 2024

View reviewed changes

Philippe-Cholet mentioned this pull request Jan 18, 2024

add test coverage workflow #847

Merged

phimuemue approved these changes Jan 22, 2024

View reviewed changes

Philippe-Cholet and others added 12 commits January 22, 2024 14:07

MultiProduct: fix documentation

d831b6e

MultiProduct: fix debug name

dbd47a2

MultiProduct: start over

32fe7f5

MultiProduct::next

113f4bb

Co-Authored-By: Jakob Degen <[email protected]>

MultiProduct: test the bugfix

e34d52b

An empty product is supposed to generate a single empty vector, test this. Co-Authored-By: Jakob Degen <[email protected]>

MultiProduct is now fused

2f288ef

MultiProduct::last

131cbbd

MultiProduct::count

9a03092

`count` is generally not a cheap operation so I avoid it when the result would be zero anyway.

MultiProduct::size_hint

da5e54c

Similar to `count` but with "size hint" operations.

MultiProduct: add some documentation

b9f22c5

MultiProduct: ends after the nullary product

059221c

While adding comments, I realized I could set the iterator as finished in the case of the nullary cartesian product. It adds a simple invariant. I thought of making a comment but a debug check is better.

Philippe-Cholet added 4 commits January 22, 2024 14:07

MultiProduct: add comments

64a5bdb

MultiProduct state: comment with code

cb21272

MultiProduct values: comment with code (1)

76984fd

MultiProduct values: comment with code (2)

085990c

Separate `Populated/NotYetPopulated` in `match` blocks. Indents change quite a bit.

Philippe-Cholet force-pushed the rewrite-multiproduct branch from d0d8d4f to 085990c Compare January 22, 2024 13:07

Philippe-Cholet removed this from the next milestone Jan 22, 2024

Philippe-Cholet mentioned this pull request Jan 25, 2024

Prefer boxed slices over vectors internally #863

Merged

Philippe-Cholet added this to the next milestone Jan 30, 2024

Philippe-Cholet added this pull request to the merge queue Jan 30, 2024

Philippe-Cholet removed this pull request from the merge queue due to a manual request Jan 30, 2024

Philippe-Cholet added this pull request to the merge queue Jan 30, 2024

Merged via the queue into rust-itertools:master with commit 33541c4 Jan 30, 2024
13 checks passed

Philippe-Cholet deleted the rewrite-multiproduct branch January 30, 2024 14:12

This was referenced Jan 30, 2024

Add documentation for multi_cartesian_product when iterator is empty #834

Merged

iproduct: generate tuples for simpler cases too #870

Merged

stdrc added a commit to risingwavelabs/risingwave that referenced this pull request Sep 30, 2024

remove hack cuz the bug is fixed by rust-itertools/itertools#835

81b9235

Signed-off-by: Richard Chien <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite `MultiProduct` #835

Rewrite `MultiProduct` #835

Philippe-Cholet commented Jan 3, 2024 •

edited

Loading

phimuemue left a comment

jswrenn commented Jan 3, 2024

Philippe-Cholet commented Jan 4, 2024 •

edited

Loading

jswrenn left a comment

codecov-commenter commented Jan 18, 2024 •

edited by codecov bot

Loading

Philippe-Cholet commented Jan 18, 2024

phimuemue left a comment

Philippe-Cholet commented Jan 22, 2024

jswrenn commented Jan 22, 2024

Philippe-Cholet commented Jan 30, 2024

Rewrite MultiProduct #835

Rewrite MultiProduct #835

Conversation

Philippe-Cholet commented Jan 3, 2024 • edited Loading

phimuemue left a comment

Choose a reason for hiding this comment

jswrenn commented Jan 3, 2024

Philippe-Cholet commented Jan 4, 2024 • edited Loading

jswrenn left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 18, 2024 • edited by codecov bot Loading

Codecov Report

Philippe-Cholet commented Jan 18, 2024

phimuemue left a comment

Choose a reason for hiding this comment

Philippe-Cholet commented Jan 22, 2024

jswrenn commented Jan 22, 2024

Philippe-Cholet commented Jan 30, 2024

Rewrite `MultiProduct` #835

Rewrite `MultiProduct` #835

Philippe-Cholet commented Jan 3, 2024 •

edited

Loading

Philippe-Cholet commented Jan 4, 2024 •

edited

Loading

codecov-commenter commented Jan 18, 2024 •

edited by codecov bot

Loading