search

clj-commons / manifold

A compatibility layer for event-driven abstractions
1.02k stars 106 forks source link

Mapcat cannot handle mapping functions that return streams? #179

Closed jdormit closed 1 year ago

jdormit commented 5 years ago

The mapcat function seems to choke if you pass in a mapping function that returns a stream instead of a sequence:

user> (s/stream->seq (s/mapcat (fn [x] (s/->source [x])) (s/->source [1 2 3])))
()
Aug 18, 2019 2:23:39 PM clojure.tools.logging$eval5577$fn__5581 invoke
SEVERE: error in message propagation
java.lang.IllegalArgumentException: Don't know how to create ISeq from: manifold.stream.random_access.RandomAccessSource
    at clojure.lang.RT.seqFrom(RT.java:550)
    at clojure.lang.RT.seq(RT.java:530)
    at clojure.core$seq__5124.invokeStatic(core.clj:137)
    at clojure.core$empty_QMARK_.invokeStatic(core.clj:6126)
    at clojure.core$empty_QMARK_.invoke(core.clj:6126)
    at manifold.stream$mapcat$fn__7845$this__6747__auto____7849$fn__7850.invoke(stream.clj:749)
    at manifold.stream$mapcat$fn__7845$this__6747__auto____7849.invoke(stream.clj:748)
    at manifold.stream$mapcat$fn__7845.invoke(stream.clj:748)
    at manifold.stream.Callback.put(stream.clj:454)
    at manifold.stream.graph$async_send.invokeStatic(graph.clj:81)
    at manifold.stream.graph$async_send.invoke(graph.clj:77)
    at manifold.stream.graph$sync_connect$f__5925__auto____7091.invoke(graph.clj:274)
    at clojure.lang.AFn.run(AFn.java:22)
    at io.aleph.dirigiste.Ex
ecutor$3.run(Executor.java:318)
    at io.aleph.dirigiste.Executor$Worker$1.run(Executor.java:62)
    at manifold.executor$thread_factory$reify__5807$f__5808.invoke(executor.clj:44)
    at clojure.lang.AFn.run(AFn.java:22)
    at java.lang.Thread.run(Thread.java:748)

However, if you express the same calculation as a composition of concat and map, it works fine:

user> (s/stream->seq (s/concat (s/map (fn [x] (s/->source [x])) (s/->source [1 2 3]))))
(1 2 3)

Am I misunderstanding the purpose of mapcat? Or should it be able to handle mapping functions that return streams?

KingMob commented 3 years ago

I'm torn on whether to treat this as a bug or not.

On the one hand, the docstring for core mapcat says "function f should return a collection", while the docstring for Manifold mapcat says it's equivalent to clj mapcat, which would exclude streams, as they're not colls.

On the other hand, that's kinda counter-intuitive to the idea that mapcat should just be (apply concat (apply map ...

On the gripping hand... Manifold concat only works with streams, not seqs, so a fn we used with core mapcat wouldn't work with (s/concat (s/map ...

I think for now, maybe a docstring update will suffice, though if you craft a PR, it would be welcomed.

cosineblast commented 1 year ago

Can we consider this issue as closed? Or should we consider further adjusting mapcat?

KingMob commented 1 year ago

@figurantpp Hmm, I left a comment on your PR saying "Closes #179", but it didn't work.

I must be missing something, but based on https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword, it seems like it should have closed this as soon as it was merged...