Clojure

Transducing an eduction finishes twice

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Environment:
    1.7.0-alpha4
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

> (transduce (map identity)
             (fn
               ([s] (println "Finishing") s)
               ([s i] s))
             nil
             (eduction (map identity) []))
Finishing
Finishing
nil

Cause: transduce passes (xf f) into .reduce of Eduction, which calls transduce, causing completing xf to be called more than once.

Proposed: Eduction reduce should use (completing f) instead of f to isolate completion of inner xf from outer xf.

Patch: CLJ-1606-5.patch

Screened by: Alex Miller

  1. CLJ-1606.patch
    27/Nov/14 11:34 PM
    2 kB
    Ghadi Shayban
  2. CLJ-1606-2.patch
    28/Nov/14 8:54 AM
    2 kB
    Alex Miller
  3. CLJ-1606-3.patch
    15/Dec/14 1:19 AM
    2 kB
    Ghadi Shayban
  4. CLJ-1606-4.patch
    15/Dec/14 8:37 AM
    2 kB
    Alex Miller
  5. CLJ-1606-5.patch
    08/Jan/15 6:24 PM
    2 kB
    Ghadi Shayban

Activity

Hide
Alex Miller added a comment -

identity is not a valid xf - changed to (map identity)

Show
Alex Miller added a comment - identity is not a valid xf - changed to (map identity)
Hide
Ghadi Shayban added a comment -

identity is a valid though nonsensical transducer. fix & test added.

Show
Ghadi Shayban added a comment - identity is a valid though nonsensical transducer. fix & test added.
Hide
Ghadi Shayban added a comment -

Simple reproduction similar to into:

(transduce (map dec)
           (completing conj! persistent!)
           (transient [])
           (eduction (map inc) (range 6)))

;; ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.ITransientCollection

into doesn't use completing, and conj! has an arity that hides the problem.

Show
Ghadi Shayban added a comment - Simple reproduction similar to into:
(transduce (map dec)
           (completing conj! persistent!)
           (transient [])
           (eduction (map inc) (range 6)))

;; ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.ITransientCollection
into doesn't use completing, and conj! has an arity that hides the problem.
Hide
Alex Miller added a comment -

I removed trailing whitespace in the patch so it applies cleanly.

Show
Alex Miller added a comment - I removed trailing whitespace in the patch so it applies cleanly.
Hide
Ghadi Shayban added a comment -

This patch is a little more subtle than I thought. Completion of the eduction's rfn needs to be handled separately from the "outer" transduce's xform. Patch coming.

Show
Ghadi Shayban added a comment - This patch is a little more subtle than I thought. Completion of the eduction's rfn needs to be handled separately from the "outer" transduce's xform. Patch coming.
Hide
Ghadi Shayban added a comment -

New patch with tests that completes the inner xform without completing the passed in rfn

Show
Ghadi Shayban added a comment - New patch with tests that completes the inner xform without completing the passed in rfn
Hide
Ghadi Shayban added a comment -

both -3 and -2 are equivalent. -3 is probably better stylistically.

Show
Ghadi Shayban added a comment - both -3 and -2 are equivalent. -3 is probably better stylistically.
Hide
Alex Miller added a comment -

Added CLJ-1606-4.patch - identical to -3, just fixed whitespace error.

Show
Alex Miller added a comment - Added CLJ-1606-4.patch - identical to -3, just fixed whitespace error.
Hide
Andy Fingerhut added a comment -

There are two identically named attachments here (containing -2). It looks like it isn't the one under consideration, but it might be nice to remove or rename to avoid the name conflict.

Show
Andy Fingerhut added a comment - There are two identically named attachments here (containing -2). It looks like it isn't the one under consideration, but it might be nice to remove or rename to avoid the name conflict.
Hide
Ghadi Shayban added a comment -

Andy, not sure how to do that, but in any case I just added -5 clarifying language in the comment

Show
Ghadi Shayban added a comment - Andy, not sure how to do that, but in any case I just added -5 clarifying language in the comment
Hide
Alex Miller added a comment -

Ghadi, that was super confusing. Did you just add a new -5 patch? The -4 patch has already been screened, and you have not removed the duplicate -2 patch so I don't get what the -5 is. Can we just delete the -5 and older -2 patches?

Show
Alex Miller added a comment - Ghadi, that was super confusing. Did you just add a new -5 patch? The -4 patch has already been screened, and you have not removed the duplicate -2 patch so I don't get what the -5 is. Can we just delete the -5 and older -2 patches?
Hide
Andy Fingerhut added a comment -

Sorry for adding to the confusion. Ghadi, instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Show
Andy Fingerhut added a comment - Sorry for adding to the confusion. Ghadi, instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches
Hide
Ghadi Shayban added a comment -

Sorry. Fine by me, though permissions prevent me from deleting one of the patches.

As I read through the screened patch I just tried to clarify the wording. This:

;; NB (completing f) isolates completion of inner xfns from outer xfns
became:
;; NB (completing f) isolates completion of inner rf from outer rf

Feel free to nix that -5 patch if that's worthless

Show
Ghadi Shayban added a comment - Sorry. Fine by me, though permissions prevent me from deleting one of the patches. As I read through the screened patch I just tried to clarify the wording. This: ;; NB (completing f) isolates completion of inner xfns from outer xfns became: ;; NB (completing f) isolates completion of inner rf from outer rf Feel free to nix that -5 patch if that's worthless
Hide
Alex Miller added a comment -

Gotcha. I will take care of the further changes later tonight.

In the future, please don't modify screened patches without letting me know.

Show
Alex Miller added a comment - Gotcha. I will take care of the further changes later tonight. In the future, please don't modify screened patches without letting me know.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: