Skip to content

implement Iter::chunk_by #2029

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

juliano
Copy link
Contributor

@juliano juliano commented Apr 28, 2025

Copy link

peter-jerry-ye-code-review bot commented Apr 28, 2025

Initial array capacity could be more memory efficient

Category
Performance
Code Snippet
let mut buffer : Array[T] = []
Recommendation
Consider using a smaller initial capacity like 4 or 8 instead of the default capacity of 16 that's set later
Reasoning
The current implementation creates an empty array first, then later replaces it with a new array of capacity 16. For small chunks (which are common in chunking operations), this may be wasteful. Starting with a smaller capacity would be more memory efficient while still allowing growth for larger chunks.

Nested callback makes control flow harder to follow

Category
Maintainability
Code Snippet
fn emit_current_chunk() -> IterResult {
if buffer.is_empty() {
return IterContinue
}
...
}
Recommendation
Consider extracting the chunk emission logic to a separate helper function outside the iterator callback
Reasoning
Having nested function definitions makes the code harder to follow and maintain. The chunk emission logic is a distinct operation that could be separated for better readability and potential reuse.

Missing documentation for edge cases

Category
Correctness
Code Snippet
/// Groups elements of an iterator according to a discriminator function.
Recommendation
Add documentation about behavior with empty iterators and when the discriminator function returns equal keys for non-consecutive elements
Reasoning
While the test cases cover these scenarios, the documentation should explicitly state how the function behaves in edge cases to help users understand the API contract.

@coveralls
Copy link
Collaborator

coveralls commented Apr 28, 2025

Pull Request Test Coverage Report for Build 6557

Details

  • 9 of 11 (81.82%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 92.494%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/iter.mbt 9 11 81.82%
Totals Coverage Status
Change from base Build 6554: -0.02%
Covered Lines: 6124
Relevant Lines: 6621

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator

I'm still not quite sure if the signature should be -> Iter[(K, Iter[V])] or -> Iter[(K, Array[V])]. Maybe we need some advices from the others here

@peter-jerry-ye peter-jerry-ye requested a review from bobzhang April 30, 2025 02:26
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