<< Back to previous view

[CLJ-1572] into does not work with IReduceInit Created: 24/Oct/14  Updated: 17/Nov/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transducers

Attachments: Text File clj-1572-2.patch     Text File clj-1572-3.patch     Text File clj-1572-4.patch     Text File CLJ-1572-alternative-POC.patch     Text File clj-1572.patch    
Patch: Code and Test
Approval: Vetted

 Description   

This should work:

(into []
  (reify clojure.lang.IReduceInit
    (reduce [_ f start]
      (reduce f start (range 10)))))
IllegalArgumentException Don't know how to create ISeq from: user$eval5$reify__6
	clojure.lang.RT.seqFrom (RT.java:506)
	clojure.lang.RT.seq (RT.java:487)
	clojure.core/seq--seq--4091 (core.clj:135)
	clojure.core.protocols/seq-reduce (protocols.clj:30)
	clojure.core.protocols/fn--6422 (protocols.clj:42)
	clojure.core.protocols/fn--6369/f--6255--auto----G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6469)
	clojure.core/into (core.clj:6550)

Cause: CollReduce only supports IReduce, not IReduceInit so when reduce calls into it, it falls back to trying to obtain a seq representation which fails.

Proposed: Extend CollReduce to IReduceInit and in the non-init arity, cast to IReduce. Also, now that CollReduce supports both IReduceInit and Iterable, a coll that implements both makes the path through CollReduce nondeterministic. transduce does an explicit check that prefers IReduceInit - the patch copies that approach to reduce as well.

Patch: clj-1572-4.patch



 Comments   
Comment by Alex Miller [ 24/Oct/14 10:40 AM ]

into calls reduce which calls into CollReduce. CollReduce extends to IReduce, but not to IReduceInit. If CollReduce were extended to IReduceInit for the arity it can support, into work as expected in the given example. Patch clj-1572.patch does this.

Comment by Ghadi Shayban [ 08/Nov/14 4:34 PM ]

It is also possible that core/reduce needs the same special casing of IReduceInit that transduce has to allow for a deterministic dispatch when transduce is called with (mapcat f), as mapcat calls reduce.

Comment by Stuart Halloway [ 10/Nov/14 11:02 AM ]

Can someone please expand on Ghadi's comment with an example of the problem?

Comment by Ghadi Shayban [ 10/Nov/14 11:14 AM ]

Example of something that is Iterable & ReduceInit:
https://github.com/ghadishayban/reducers/blob/master/src/ghadi/reducers.clj#L122-L128

Let's call that r/range in this example:
(transduce (mapcat r/range) + 0 [5 5 5 5 5])

The when the mapcat transducer encounters r/range, the inner reduce call will dispatch through CollReduce upon Iterable, rather than IReduceInit.

the inner call to reduce within cat:
https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7243

Comment by Alex Miller [ 12/Nov/14 12:55 PM ]

To restate the issue from Ghadi for my own sake:

The CollReduce protocol extends to IReduce, IReduceInit and Iterable. Because these are all interfaces, its possible for a custom coll to implement two or more of them. In that case, Clojure will arbitrarily pick which protocol impl is called - this can result in the Iterable version being called instead of IReduce/IReduceInit (which should be preferred).

transduce avoids this by explicitly checking for IReduceInit and preferring it over CollReduce.

Ghadi is suggesting that reduce should also make this preference (currently it does not).

Comment by Nicola Mometto [ 17/Nov/14 3:06 PM ]

If CollReduce could be direcly backed by the IReduce interface, this would remove the need for explicit IReduceInit checking at the callsite.

It's already possible to (defprotocol CollReduce :on-interface clojure.lang.IReduce ..), I'm proposing adding the ability to map the "reduce" method to the coll-reduce protocol-fn aswell and go with this solution

Comment by Alex Miller [ 17/Nov/14 3:21 PM ]

CollReduce extends to two interfaces (IReduceInit and Iterable) and for some impls this is ambiguous under the CollReduce protocol. The check in reduce and transduce is to force the choice of IReduceInit so it is not ambiguous. I think your suggestion re-introduces that issue? Or maybe I'm just not understanding what you mean.

Comment by Nicola Mometto [ 17/Nov/14 3:46 PM ]

Turns out defprotocol already has that capability via :on metadata field.

The attached patch is a proof of concept of my proposal, if there's interest in this approach I can fix the deftype/record/reify method parser to automatically pick the var name rather than having to specify the method name.

Comment by Nicola Mometto [ 17/Nov/14 3:52 PM ]

Ah, I see now the issue. Disregard my patch then.





[CLJ-1589] Cleanup internal-reduce implementation Created: 14/Nov/14  Updated: 15/Nov/14

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-cleanup-internal-reduce-impl.patch    
Patch: Code
Approval: Vetted

 Description   

Currently internal-reduce provides an implementation for ArraySeq and the ArraySeq_* prim classes.
Since those classes implement IReduce the current patch makes instances of those classes fallback on coll-reduce's IReduce impl (that simply invokes .reduce)

This change is desiderable because it removes unnecessary duplicated code, reducing the implementation surface and making it easier to follow reduce's code path.

This issue depends on the "clj-1537-gvec-ArraySeq.patch" patch at http://dev.clojure.org/jira/browse/CLJ-1590 since the current IReduce impl for those ArraySeq classes doesn't properly handle Reduced



 Comments   
Comment by Alex Miller [ 14/Nov/14 10:28 PM ]

I'm not sure whether this should be in 1.7 or not, but I'm adding it there so we can have a discussion on it regardless.





Generated at Sat Nov 22 18:09:19 CST 2014 using JIRA 4.4#649-r158309.