Skip to content
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

refactor: UpdateDistinct and Duplicate operations. #168

Merged
merged 3 commits into from
Aug 1, 2021

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Jul 31, 2021

This PR:

  • Align the Duplicate operation on Distinct
  • Update the Distinct operation
    The distinct operation was not designed in a lazy way.
    The previous version was storing in an array each pair of key-value from the iterator. This is not how we should do that.
    The previous version was based on Reduce and was traversing the whole iterator before going to the next operation.
    This is not really optimal and we should update that.
  • Has updated documentation and examples
  • The Duplicate implementation is ready and I'm happy of it

Because of the nature of these operations and their side effects (the $stack variable), I'm unable to test them completely, especially the continue 2 statement. Infection will most probably detect that without any trouble (Basically, there is no need to go through the whole $seen array once the inner condition is met).
I haven't found a better way to achieve this, maybe the enlightenment will come another day.

@drupol drupol added the enhancement New feature or request label Jul 31, 2021
@drupol drupol requested a review from AlexandruGG July 31, 2021 20:55
@drupol drupol marked this pull request as draft July 31, 2021 21:36
@drupol drupol force-pushed the refactor/improve-distinct-operation branch from 11aec79 to a1ac585 Compare August 1, 2021 06:24
@drupol drupol changed the title refactor: Optimize Distinct operation. refactor: UpdateDistinct and Duplicate operations. Aug 1, 2021
@drupol drupol marked this pull request as ready for review August 1, 2021 06:42
@drupol drupol enabled auto-merge (squash) August 1, 2021 06:42
@drupol drupol marked this pull request as draft August 1, 2021 06:58
auto-merge was automatically disabled August 1, 2021 06:58

Pull request was converted to draft

@drupol drupol self-assigned this Aug 1, 2021
@drupol drupol force-pushed the refactor/improve-distinct-operation branch 2 times, most recently from 01ff5a6 to 3db3965 Compare August 1, 2021 08:17
@drupol drupol force-pushed the refactor/improve-distinct-operation branch from 3db3965 to 032aff8 Compare August 1, 2021 08:19
@drupol drupol marked this pull request as ready for review August 1, 2021 08:21
@drupol drupol enabled auto-merge (squash) August 1, 2021 08:21
@drupol drupol removed their assignment Aug 1, 2021
Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍 .

This would be the kind of refactoring that would really benefit from having PHPBench so we could see the difference in runtime and memory used by the operations.

@drupol
Copy link
Collaborator Author

drupol commented Aug 1, 2021

Nice one +1 .

This would be the kind of refactoring that would really benefit from having PHPBench so we could see the difference in runtime and memory used by the operations.

I definitely agree.

That said, I did the tests locally and the difference is significant!

@drupol drupol disabled auto-merge August 1, 2021 19:10
@drupol drupol merged commit 98e1f74 into master Aug 1, 2021
@drupol drupol deleted the refactor/improve-distinct-operation branch August 1, 2021 19:10
@AlexandruGG
Copy link
Collaborator

Nice one +1 .
This would be the kind of refactoring that would really benefit from having PHPBench so we could see the difference in runtime and memory used by the operations.

I definitely agree.

That said, I did the tests locally and the difference is significant!

I'll find the time at some point to play around with phpbench and see how we introduce it into this project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants