eclipse-collections icon indicating copy to clipboard operation
eclipse-collections copied to clipboard

Implement LazyIterable.collectWithIndex

Open victornoel opened this issue 5 years ago • 9 comments

Hi,

I noticed that LazyIterable does not have a collectWithIndex method, is that on purpose or would it make sense to have it implemented?

victornoel avatar Sep 17 '20 12:09 victornoel

From my recollection that is on purpose as the index can’t be determined for the subsequent operations. So if it’s a terminating method it makes sense to add. But I may be missing something.

nikhilnanivadekar avatar Sep 17 '20 13:09 nikhilnanivadekar

@nikhilnanivadekar there is zipWithIndex, isn't it the same in terms of execution? But instead of returning a Pair<T, Integer>, we just get a T from collectWithIndex… (I'm not sure ^^)

victornoel avatar Sep 17 '20 14:09 victornoel

Ah! Good point! Tagging @motlin and @donraab to know if there is any historical context for it. If not I’ll say we just didn’t get around to implement it 😄

nikhilnanivadekar avatar Sep 17 '20 14:09 nikhilnanivadekar

You know how ParallelIterable has container-specific sub-interfaces? Like ParallelListIterable.

For a long time, I've wanted to add container-specific sub-interfaces to LazyIterable. It's a big project though and I haven't had time.

If we had such interfaces, the WithIndex methods would go on the ordered containers like lazy lists and lazy stacks. Today, when we add these methods, they apply to hash tables like UnifiedSet, which I think is awkward.

Anyway, that's why I've never added the method. But it would be fine to go ahead and add.

motlin avatar Sep 17 '20 15:09 motlin

@motlin covered most of what I would have said.

This method in the context of LazyIterable is also slightly more complicated because the underlying collection may not just be unordered, but you may be looking at a pipeline of operations. So if you have calls to select before collectWithIndex, even if you are iterating over an ordered collection, you are not looking at the indices, you are looking at the current count in the iteration.

That said, as long as it is well documented and easy to understand in the javadoc and you have use cases for this @victornoel I would be fine with having it added.

donraab avatar Oct 02 '20 16:10 donraab

@donraab yes, good point about indices vs count: I was indeed expecting the count to be used, exactly because of what you said, in a lazy iterable context, that's the only thing that would make sense.

I have a use case, basically it would avoid using an intermediate zipWithIndex() and passing around a LazyIterable<Pair<T, Integer>> with unboxed int. I'm not sure it has a real big impact on performance (I have thousands of objects each holding such iterable and repetitively executing this code), but it would at least make the code simpler to read :)

victornoel avatar Oct 02 '20 16:10 victornoel

By the way it would also make sense to add collectWithIndex (as well as zipWithIndex if desired) to the primitive lazy iterables I believe but maybe it will mean to derive this method for each return type possible though…

victornoel avatar Oct 02 '20 16:10 victornoel

There is an interesting problem that exists with zipWithIndex that could be corrected on the primitive side that we couldn't fix on the Object side. We had zipWithIndex implemented on the Object side before we had support for primitive containers and primitive pairs. It should return ObjectIntPair. I just looked, we don't have zipWithIndex on the primitive list side yet either. It should be added there first.

donraab avatar Oct 02 '20 17:10 donraab

@donraab btw, to fix this on the object side, wouldn't it be enough for backward compatibility to have ObjectIntPair<T> also implement Pair<T, Integer> and then have zipWithIndex use ObjectIntPair<T>? Not sure it will work though with all the covariance limitations of Java...

victornoel avatar Oct 11 '20 10:10 victornoel