loophp / collection

A (memory) friendly, easy, lazy and modular collection class.
https://loophp-collection.rtfd.io/
MIT License
721 stars 35 forks source link

Issue with distinct #341

Closed chedkowski-leonsoftware closed 4 months ago

chedkowski-leonsoftware commented 5 months ago

Steps required to reproduce the problem

I have two arrays:

$base_list = Collection::fromIterable([1, 2]);
$excluded_list = Collection::fromIterable([2])->distinct();

And I want to efficiently remove elements from the base list using the second list:

$excluded_map = $excluded_list->associate(
    callbackForKeys: static fn (int $key, $nid) => $nid,
    callbackForValues: static fn ($value) => $value,
);

$result = $base_list->filter(static fn ($row) => $excluded_map->get($row) === null);

Expected Result

Actual Result

Version

I found that something is wrong with ->distinct(). When I remove it, the code above works fine.

It seems to me that 'distinct' shouldn't have an impact on the outcome in this case. Am I mistaken 😉?

drupol commented 5 months ago

Hi,

Thanks for the issue and taking the time to provide a quick reproducer. The issue might be in the get operation, I need to make further check as soon as I have some time.

To fix the issue gracefully, I would use diff:

$result = $base_list->diff(...$excluded_list);
chedkowski-leonsoftware commented 5 months ago

In my case, i start with a list of integers that are mapped to value objects:

$excluded_list = Collection::fromIterable([2])
    ->distinct()
    ->map(fn ($v) => new ValueObject($v));

later comes the filtering logic.

Currently, I've managed to "bypass" the lazy mechanism by creating a new collection

Collection::fromIterable($excluded_list->all());

after using distinct, this helps in my case.

Thanks for the quick response!

EDIT: Sorry, I accidentally closed this issue

github-actions[bot] commented 5 months ago

Since this issue 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.