Clojure

Transducer of partition-by over take gives wrong answer

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Approval:
    Ok

Description

(partition-by pos? (take 2 [-1 1]))
=> ((-1) (1))
(sequence (comp (take 2) (partition-by pos?)) [-1 1])
=> ([-1])

Activity

Hide
Nicola Mometto added a comment -

Given that it works fine when using transduce instead of sequence, the bug might be in LazyTransformer rather than in partition-by.

(into [] (comp (take 2) (partition-by pos?)) [-1 1])
=> [[-1] [1]]
Show
Nicola Mometto added a comment - Given that it works fine when using transduce instead of sequence, the bug might be in LazyTransformer rather than in partition-by.
(into [] (comp (take 2) (partition-by pos?)) [-1 1])
=> [[-1] [1]]
Hide
Ghadi Shayban added a comment -

Patch fixes the test case, but needs eyes, I certainly may have broken something. This highlights the importance of CLJ-1554, something similar to the existing defequiv tests for reducers, but between #'into and #'sequence, also covering edge cases in reduced unwrapping.

Show
Ghadi Shayban added a comment - Patch fixes the test case, but needs eyes, I certainly may have broken something. This highlights the importance of CLJ-1554, something similar to the existing defequiv tests for reducers, but between #'into and #'sequence, also covering edge cases in reduced unwrapping.
Hide
Alex Miller added a comment -

Thanks Ghadi. This bug was found by the tests I wrote for CLJ-1554, so yes.

Show
Alex Miller added a comment - Thanks Ghadi. This bug was found by the tests I wrote for CLJ-1554, so yes.
Hide
Nicola Mometto added a comment -

Applying this patch causes a regression in the lazyiness of sequence.
The lines that Ghadi removed for this patch were added by Rich in this commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c to address http://dev.clojure.org/jira/browse/CLJ-1497

Example of the regression:
current master:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
(1 2)

with this patch:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
~3
(1 2)
Show
Nicola Mometto added a comment - Applying this patch causes a regression in the lazyiness of sequence. The lines that Ghadi removed for this patch were added by Rich in this commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c to address http://dev.clojure.org/jira/browse/CLJ-1497 Example of the regression: current master:
user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
(1 2)
with this patch:
user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
~3
(1 2)
Hide
Nicola Mometto added a comment -

Patch 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch addresses this issue while preserving the current lazyness factor of `sequence`

Show
Nicola Mometto added a comment - Patch 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch addresses this issue while preserving the current lazyness factor of `sequence`
Hide
Alex Miller added a comment -

Rich has a (different) patch for this on the way.

Show
Alex Miller added a comment - Rich has a (different) patch for this on the way.
Hide
Alex Miller added a comment -
Show
Alex Miller added a comment - Fixed directly by Rich in commit https://github.com/clojure/clojure/commit/38d7572e4254afdd7f02b78095ccdb27065754d2

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: