implement Iter::group_by

[juliano]

Fixes #1201

[peter-jerry-ye-code-review]

Potentially inefficient array allocation in chunk_by

Category
Performance
Code Snippet
buffer = Array::new(capacity=16)
Recommendation
Consider using a dynamic capacity based on input size or previous chunk sizes
Reasoning
Using a fixed capacity of 16 for the buffer array might be inefficient for large chunks. If chunks are typically larger, this will cause multiple reallocations. If chunks are typically smaller, this wastes memory.

Nested closure complexity in chunk_by implementation

Category
Maintainability
Code Snippet
fn(yield_) {
let mut has_current = false
let mut current_key : K? = None
let mut buffer : Array[T] = []
fn emit_current_chunk() -> IterResult { ... }
Recommendation
Consider splitting the implementation into smaller functions or using a struct to hold state
Reasoning
The nested closure structure with mutable state makes the code harder to follow. Using a struct to encapsulate the chunking state would make the code more maintainable.

Potential memory leak in chunk_by error handling

Category
Correctness
Code Snippet
if emit_current_chunk() == IterEnd {
return IterEnd
}
Recommendation
Add cleanup code to ensure buffer is cleared when iteration ends early
Reasoning
If iteration ends early due to IterEnd, the remaining buffer contents might not be properly cleaned up. Consider adding a cleanup mechanism or documenting this behavior.

[coveralls]

Pull Request Test Coverage Report for Build 6456

Details

• 12 of 14 (85.71%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 92.67%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
builtin/iter.mbt	12	14	85.71%

Totals
Change from base Build 6455:	0.0%
Covered Lines:	6132
Relevant Lines:	6617

---

💛 - Coveralls

[hackwaly]

Why there's two 30? It's not expected from users.

[juliano]

It's the same principle as the test above, it's generating a new group every time the value of the key function changes.

[peter-jerry-ye]

I have doubt about the type signature. I'm not quite sure whether we should have Iter inside Iter. It looks kind of weird to me. Maybe Iter[(Key, Array[Value])] is enough. I'm also not certain with the semantics. Are we expecting group_by to be an iterator that merges the adjacent items or are we expecting it to group everything visited, in which case we should have Map[Key, Array[Value]] as result.

[hackwaly]

I think Map[Key, Array[Value]] make sense

[juliano]

This one was my reference for the implementation, but I probably should've questioned beforehand :)

[peter-jerry-ye]

What I had in mind was

• this: https://learn.microsoft.com/en-us/dotnet/csharp/linq/standard-query-operators/grouping-data
• and this: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/stream/Collectors.html#groupingBy(java.util.function.Function)

Seems we all have different understanding of what group by means

[juliano]

It was actually my initial thought, but after reading a bit about iterators, that approach made sense.

No problem though, will come up with the new version soon

[peter-jerry-ye]

FTR

The original implementation may be renamed with a more proper name such as:

• https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.chunk_by
• https://clojuredocs.org/clojure.core/partition-by
• https://ruby-doc.org/3.4.1/Enumerable.html#method-i-chunk
• https://hexdocs.pm/elixir/1.12/Enum.html#chunk_by/2

Though some uses group_by:

• https://hackage.haskell.org/package/base-4.21.0.0/docs/Data-List.html#v:groupBy
• https://docs.python.org/3/library/itertools.html#itertools.groupby

Also notice that

• chunk are usually considered as split into containers that has the fixed size
• partition are usually considered as splitting into 2

[peter-jerry-ye]

For better performance, we may need #1965

[juliano]

FTR

The original implementation may be renamed with a more proper name such as:

• https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.chunk_by
• https://clojuredocs.org/clojure.core/partition-by
• https://ruby-doc.org/3.4.1/Enumerable.html#method-i-chunk
• https://hexdocs.pm/elixir/1.12/Enum.html#chunk_by/2

Though some uses group_by:

• https://hackage.haskell.org/package/base-4.21.0.0/docs/Data-List.html#v:groupBy
• https://docs.python.org/3/library/itertools.html#itertools.groupby

Also notice that

• chunk are usually considered as split into containers that has the fixed size
• partition are usually considered as splitting into 2

It seems to me that either partition_by and chunk_by are appropriate. Any preferences @peter-jerry-ye @hackwaly ?

[hackwaly]

FTR
The original implementation may be renamed with a more proper name such as:

• https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.chunk_by
• https://clojuredocs.org/clojure.core/partition-by
• https://ruby-doc.org/3.4.1/Enumerable.html#method-i-chunk
• https://hexdocs.pm/elixir/1.12/Enum.html#chunk_by/2

Though some uses group_by:

• https://hackage.haskell.org/package/base-4.21.0.0/docs/Data-List.html#v:groupBy
• https://docs.python.org/3/library/itertools.html#itertools.groupby

Also notice that

• chunk are usually considered as split into containers that has the fixed size
• partition are usually considered as splitting into 2

It seems to me that either partition_by and chunk_by are appropriate. Any preferences @peter-jerry-ye @hackwaly ?

There's chunk_by in core. The previous implementation should be called chunk_by. LGTM for the new implementation.

[peter-jerry-ye]

Can we keep the group_by for this PR and have chunk_by in another PR? I still have some doubt about the signature.