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

fix: allow inner loop traversals of SimpleCachingIteratorAggregate #50

Closed
wants to merge 6 commits into from

Conversation

rela589n
Copy link

This PR adds consecutive caching to SimpleCachingIteratorAggregate allowing to traverse the same iterator within the loop without affecting outer iteration.

See #49

@rela589n rela589n requested a review from drupol as a code owner January 14, 2024 17:04
Copy link

what-the-diff bot commented Jan 14, 2024

PR Summary

  • Update in Caching Mechanism
    At the core of the pull request are the changes made in the SimpleCachingIteratorAggregate.php file. This file has been updated to boost its efficiency when processing data. Specifically, it now does an additional check to continue its operations if it encounters the last data point in its memory. As part of this update, it includes a feature that allows it to keep track of previously processed data. This change will significantly enhance the system's performance by optimizing the data processing techniques.

  • Enhancement in Test Procedures
    Alongside the above changes, this PR also enhances the capabilities of our testing procedures. A new testing method, aptly named testConsecutiveCachingFromInnerTraversals, has been added to the SimpleCachingIteratorAggregateTest.php file. This method is in place to verify that the updated data caching mechanism works accurately, especially when dealing with complex data structures. It helps ensure that our deliverables are reliable and efficient.

@drupol
Copy link
Contributor

drupol commented Jan 15, 2024

Thanks for your pull request, I will review it carefully as soon as I have a bit of time.

@drupol
Copy link
Contributor

drupol commented Jan 19, 2024

I've updated a bit the PR, do you think we could do something to avoid the if() in the foreach ?

@drupol
Copy link
Contributor

drupol commented Jan 19, 2024

I found a case where this PR would not be valid, here it is:

<?php

declare(strict_types=1);

namespace Issue49;

use Generator;
use loophp\iterators\SimpleCachingIteratorAggregate;

require __DIR__ . '/vendor/autoload.php';

$input = static function (): Generator {
    yield 'a' => 1;
    yield 'a' => 2;
    yield 'a' => 3;
    yield 'a' => 4;
    yield 'a' => 5;
    yield 'a' => 6;
};

$iter = new SimpleCachingIteratorAggregate($input());

foreach ($iter as $ko => $vo) {
    if ($ko === 2) {
        foreach ($iter as $ki => $vi) {
            var_dump("inner", $ki, $vi);
        }
    }

    var_dump("outer", $ko, $vo);
}

The issue lies in the fact that we rely on the value of the key. Since there can be multiple same keys in an iterator, we need to find another way to achieve this if you want this PR to be merged.

Copy link

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Jan 25, 2024
@rela589n
Copy link
Author

I found a case where this PR would not be valid
...
The issue lies in the fact that we rely on the value of the key. Since there can be multiple same keys in an iterator, we need to find another way to achieve this if you want this PR to be merged.

I'm not sure what is the expected behavior thereon.

I have just added testSecondTraversalEliminatesDuplicatedKeys() that passes regardless of current-PR changes and generally represents how SimpleCachingIteratorAggregate deals with duplicated keys on consecutive traversals.

These two statements accurately represent it:

self::assertSame([
    ['a', 1],
    ['a', 2],
    ['a', 3],
], $firstIterationItems);

self::assertSame([
    ['a', 3],
], $secondIterationItems);

That being said, current logic of inner loop iterator traversal, looks expected to me:

public function testInnerTraversalConsumesDuplicatedKeyValues(): void
{
    $input = static function (): Generator {
        yield 'a' => 1;
        yield 'a' => 2;
        yield 'a' => 3;
        yield 'b' => 4;
        yield 'b' => 5;
    };

    $iter = new SimpleCachingIteratorAggregate($input());

    $outerItems = $innerItems = [];

    foreach ($iter as $ko => $vo) {
        if ($vo === 2) {
            foreach ($iter as $ki => $vi) {
                $innerItems[] = [$ki, $vi];
            }
        }

        $outerItems[] = [$ko, $vo];
    }

    self::assertSame([
        ['a', 1],
        ['a', 2],
        ['b', 5],
    ], $outerItems);

    self::assertSame([
        ['a', 2],
        ['a', 3],
        ['b', 4],
        ['b', 5],
    ], $innerItems);
}

Please, let me know what you think on this matter.

Thank you!

@github-actions github-actions bot removed the stale label Jan 28, 2024
Copy link

github-actions bot commented Feb 2, 2024

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Feb 2, 2024
@rela589n
Copy link
Author

rela589n commented Feb 3, 2024

Hi there!

Just in case you don't like the idea behind suggested changes or the implementation, please let me know so that current PR may be closed.

Thank you

@drupol
Copy link
Contributor

drupol commented Feb 3, 2024

Hi !

I haven't had the time to review it yet... This weekend with the Fosdem it's not going to be possible. Planning to do it next week hopefully!

I'll keep you posted, no worries.

Cheers.

@github-actions github-actions bot removed the stale label Feb 3, 2024
Copy link

github-actions bot commented Feb 8, 2024

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Feb 8, 2024
@github-actions github-actions bot closed this Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants