viewducers icon indicating copy to clipboard operation
viewducers copied to clipboard

View now extends GenTraversableOnce.

Open jsuereth opened this issue 10 years ago • 4 comments

This should allow views to be used inside flatMap operations. We assume this is ok because GenTraversaleOnce has NO implementations of any methods, so we need to always provide our own implementations.

TODO - We should check to see if GenTraversable[E] makes more sense to extend. That one has a few issues in terms of what it assumes (e.g. repr).

Review/Comments/Thoughts by @DarkDimius @ichoran @sethtisue

jsuereth avatar Sep 13 '15 14:09 jsuereth

This change solves the problem of signature of flatMap in current moment. But it introduces strong coupling between this library and standard collections.

We already have several other implementations of collections: https://github.com/paulp/psp-std/tree/master/std/src/main/scala and several people among us have been doing smaller experiments with changing the collection framework. I would prefer to not bind new views to current collections.

This is why I believe that in long term proposal:

def flatMap[C, U](f: El => U)(implicit viewable: IsViewable[U, C]): View[C]

is better, as it simply works with Java Streams, standard collections, @paulp collections. It would also work with off-heap arrays https://github.com/densh/scala-offheap. It does not need to be modified to support any new collections libraries, be it standard or non-standard.

Unlike this, using GenTraversaleOnce binds us to current collections hierarchy.

If somebody whats to integrate with this Views library, he will need to provide a conversion from his collection types to GenTraversaleOnce, which could be a no-go. Some collections naturally do not support size, some collections support either of foldLeft and foldRight.

Even in this PR we have a big problem created by integrating with GenTraversaleOnce: it locks us into supporting foldRight, that is implemented by converting to a Buffer, which is than converted to Stream and foldRight on it.

Note that this also breaks idea of making forcing methods obvious to see from code, as most common methods will be inherited. No _! possible.

DarkDimius avatar Sep 14 '15 13:09 DarkDimius

@jsuereth @DarkDimius Making views work with off-heap arrays is a complicated matter due to the fact that there is no such thing as instance of offheap.Array[T] as that would imply that one can store references from off-heap to on-heap which is not allowed. I wouldn't claim that either of the designs makes support for views on off-heap data structures easy.

densh avatar Sep 15 '15 16:09 densh

@DarkDimius SO. to your points ->

  1. This is just for evaluation. I'm mostly just seeking help figuring out tradeoffs, which you've done elegantly, so thank you. I'll try to play devil's advocated, but I'm not for/against this change right now, I'm still on the fence over what to do.
  2. Regarding "foldRight" support, if you look at how TraversableView is implemented, this borrows its implementation from that. We're no worse than TraversableView in our implementation, but this is bad when dealing with something that can support foldRight or SHOULD. RElated, Martin has a gist that implements a transducer stack that WOULD support foldRight. The issue are "slicing" operations aren't very foldRight conducive to begin with. Added to all that, you need to check the design intaention of "Anythign you could do x.view.<stuff>.force should also be possible with x.stagedView.<stuff>.force. I'm more than happy to lift that restriction from the design for instances like this IF it means we can still deprecate existing views.
  3. GenTraversableOnce is a single interface and at the very bottom of the stack. We are not tied to anythign except the values in it. I agree that your proposla is far more flexible. The only reason to prefer this approach is if we want to support views INSIDE flatMap against vanilla collections. I think we need to make an informed decision if we throw out that use case, and to do so I wanted to push this idea as far as I felt reasonable to make a decision (i.e. this PR).

SO, I understand the hesitation to lock down on anything out of hte current collection API. GenTraversableOnce has a lot more than I'd want, but a lot less than I original thought. It's a decent tradeoff that I think we should have a discussion about. I appreciate (and agree ALMOST fully) with your points, but wanted to see if you'd consider some of these points as well.

jsuereth avatar Sep 16 '15 20:09 jsuereth

@jsuereth I do agree that if we need to extend current collection framework than GenTraversableOnce is probably the best choice.

DarkDimius avatar Sep 16 '15 20:09 DarkDimius