Clojure

Make reducer/folder support reduce-kv

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code

Description

Currently, although rfn makes an effort to support reduce-kv, it is wasted effort since the return values of reducer and folder don't satisfy IKVReduce.

I have a working patch for this, it's quite small. I have printed a CA but not sent it, so I won't reveal the patch yet...

This also applies to ClojureScript.

Activity

Hide
Tom Jack added a comment -

Hmm.. it's not quite as simple as I thought. My first patch simply had reducer/folder implement IKVReduce with `(clojure.core.protocols/kv-reduce coll (xf f1) init)`, but for map and mapcat, this results in strange behavior (reduce-kv reducefs being called with only 2 args).

Patch 0001 includes modifications to map and mapcat so that the reduce-kv reducef will always be called with 3 args. The previous implementation of mapcat also had the converse problem: during non-kv reduce, if the mapcat fn returned a map, the reducef would be called with 3 args since maps reduce with reduce-kv.

The patch gives behavior like:

user> (->> {1 {2 3 4 5} 6 {7 8 9 10}}
        (r/mapcat #(r/map + %2))
        (reduce-kv #(conj %1 [%2 %3]) []))
[[2 5] [4 9] [7 15] [9 19]]
user> (->> [{2 3 4 5} {7 8 9 10}]
        (r/mapcat identity)
        (r/reduce conj []))
[[2 3] [4 5] [7 8] [9 10]]
Show
Tom Jack added a comment - Hmm.. it's not quite as simple as I thought. My first patch simply had reducer/folder implement IKVReduce with `(clojure.core.protocols/kv-reduce coll (xf f1) init)`, but for map and mapcat, this results in strange behavior (reduce-kv reducefs being called with only 2 args). Patch 0001 includes modifications to map and mapcat so that the reduce-kv reducef will always be called with 3 args. The previous implementation of mapcat also had the converse problem: during non-kv reduce, if the mapcat fn returned a map, the reducef would be called with 3 args since maps reduce with reduce-kv. The patch gives behavior like:
user> (->> {1 {2 3 4 5} 6 {7 8 9 10}}
        (r/mapcat #(r/map + %2))
        (reduce-kv #(conj %1 [%2 %3]) []))
[[2 5] [4 9] [7 15] [9 19]]
user> (->> [{2 3 4 5} {7 8 9 10}]
        (r/mapcat identity)
        (r/reduce conj []))
[[2 3] [4 5] [7 8] [9 10]]
Hide
Tom Jack added a comment -

Would like to discuss these changes (and auto-kv for maps) on clojure-dev, but my membership is still pending. My CA has been received. Anyone know who to contact to get my membership approved?

Show
Tom Jack added a comment - Would like to discuss these changes (and auto-kv for maps) on clojure-dev, but my membership is still pending. My CA has been received. Anyone know who to contact to get my membership approved?
Hide
Andy Fingerhut added a comment -

Tom, your patch 0001-reduce-kv-transformations.diff dated Sep 14 2012 no longer applies cleanly to the latest master, because patch lazy-rmapcat2.diff for ticket CLJ-1160 was committed to Clojure master on Aug 14 2013, and they both modify some of the same code. I haven't tried to determine whether their changes are harmonious with each other or not. I'd recommend taking a look.

Show
Andy Fingerhut added a comment - Tom, your patch 0001-reduce-kv-transformations.diff dated Sep 14 2012 no longer applies cleanly to the latest master, because patch lazy-rmapcat2.diff for ticket CLJ-1160 was committed to Clojure master on Aug 14 2013, and they both modify some of the same code. I haven't tried to determine whether their changes are harmonious with each other or not. I'd recommend taking a look.
Hide
Tom Jack added a comment -

I later realized that there are more issues with my goal (of supporting reduce-kv for the reducer combinators). I think I worked out the way I'd want things to be, but it would've involve backwards-incompatible changes to the way maps reduce. Short of that, the best thing I can think to do is probably just to not support reduce-kv at all (coincidentally, this is exactly what clojure.core.reducers does! . So as far as I'm concerned this issue is resolved. My patch certainly should not be applied, at least...

Show
Tom Jack added a comment - I later realized that there are more issues with my goal (of supporting reduce-kv for the reducer combinators). I think I worked out the way I'd want things to be, but it would've involve backwards-incompatible changes to the way maps reduce. Short of that, the best thing I can think to do is probably just to not support reduce-kv at all (coincidentally, this is exactly what clojure.core.reducers does! . So as far as I'm concerned this issue is resolved. My patch certainly should not be applied, at least...
Hide
Alex Miller added a comment -

Closed per comment

Show
Alex Miller added a comment - Closed per comment

People

  • Assignee:
    Unassigned
    Reporter:
    Tom Jack
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: