From f0a241a2de68ed83587b000613bff77b938c54bc Mon Sep 17 00:00:00 2001
From: Milan Bouchet-Valat <nalimilan@club.fr>
Date: Wed, 2 Aug 2017 16:51:37 +0200
Subject: [PATCH] Fix mapreduce() in a corner case

mapreduce_seq_impl() does not exist in Base for a long time, but this went
unnoticed because since that code path is only used in very special conditions.
This bug probably comes from Base, where the code called the seq variant when
the pairwise one should rather have called itself recursively: also change that
for the path where there are NAs.
Another consequence of this bug is that the return type of
mapreduce_pairwise_impl_skipna() was inferred as Any.
Add a test which would have triggered the failure.

Also rename a variable to fix a type instability due to using it for two
different things.
---
 src/reduce.jl  | 8 ++++----
 test/reduce.jl | 6 ++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/reduce.jl b/src/reduce.jl
index 015db0f..cfb3c01 100644
--- a/src/reduce.jl
+++ b/src/reduce.jl
@@ -22,8 +22,8 @@ function mapreduce_seq_impl_skipna(f, op, T, A::DataArray, ifirst::Int, ilast::I
 
     while i < ilast
         i += 1
-        @inbounds na = Base.unsafe_bitgetindex(chunks, i)
-        na && continue
+        @inbounds na_el = Base.unsafe_bitgetindex(chunks, i)
+        na_el && continue
         @inbounds d = data[i]
         v = op(v, f(d))
     end
@@ -36,8 +36,8 @@ function mapreduce_pairwise_impl_skipna{T}(f, op, A::DataArray{T}, bytefirst::In
         ifirst = 64*(bytefirst-1)+1
         ilast = min(64*bytelast, length(A))
         # Fall back to Base implementation if no NAs in block
-        return ilast - ifirst + 1 == n_notna ? Base.mapreduce_seq_impl(f, op, A.data, ifirst, ilast) :
-                                               mapreduce_seq_impl_skipna(f, op, T, A, ifirst, ilast)
+        return ilast - ifirst + 1 == n_notna ? Base.mapreduce_impl(f, op, A.data, ifirst, ilast) :
+                                               mapreduce_pairwise_impl_skipna(f, op, T, A, ifirst, ilast)
     end
 
     # Find byte in the middle of range
diff --git a/test/reduce.jl b/test/reduce.jl
index a30384b..e73133f 100644
--- a/test/reduce.jl
+++ b/test/reduce.jl
@@ -76,6 +76,12 @@ end
         @test sum(da2; skipna=true) ≈ sum(dropna(da2))
     end
 
+    # Check that mapreduce with skipna=true works when a full block has no NA
+    da = DataArray(1:2049)
+    da[1] = NA
+    @test isna(sum(da))
+    @test sum(da, skipna=true) === 2100224
+
     ## other reductions
 
     _varuc(x; kw...) = var(x; corrected=false, kw...)