add Base.length(::SkipMissing) for AbstractArray

[thazhemadam]

Add Base.length definition for SkipMissing.

Resolves #42672.

Before this change:

julia> x = [missing, 1, missing];

julia> (length∘skipmissing)(x)
ERROR: MethodError: no method matching length(::Base.SkipMissing{Vector{Union{Missing, Int64}}})

With this change:

julia> x = [missing, 1, missing];

julia> (length∘skipmissing)(x)
1

[martinholters]

Apart from the potentially unexpected runtime, this seems problematic as it means that calling length can actually change the length (think stateful iterators).

[thazhemadam]

this seems problematic as it means that calling length can actually change the length (think stateful iterators).

@martinholters could you please elaborate? Calling length on the same object multiple times seems to return the same result for me and the original data structure (upon which length∘skipmissing was called) also seems to be unchanged.

[martinholters]

julia> itr = Iterators.Stateful([missing, 1, missing])
Base.Iterators.Stateful{Vector{Union{Missing, Int64}}, Union{Nothing, Tuple{Union{Missing, Int64}, Int64}}}(Union{Missing, Int64}[missing, 1, missing], (missing, 2), 0)

julia> count(!ismissing, skipmissing(itr).x) # proposed definition of length
1

julia> count(!ismissing, skipmissing(itr).x)
0

[thazhemadam]

Ah, I see. The only way I can think of would be to call count on a copy instead, but that's just even less performant. 😕

[bkamins]

Maybe we could restrict the signature to skipmissing on AbstractArray. This method is really needed (otherwise there is no easy way to check the number of elements of passed object that is skipmissing).

[nalimilan]

I agree. The operation is O(N) but that's also the case for strings so I don't see that as an argument not to implement it.

[bkamins]

Looks good to me.

@thazhemadam - when this PR gets merged could you also open a follow-up PR to Compat.jl to ensure backward compatibility? Thank you!

[thazhemadam]

@thazhemadam - when this PR gets merged could you also open a follow-up PR to Compat.jl to ensure backward compatibility? Thank you!

Yes, I can do that. 🙂

[vtjnash]

Contractually, length should probably be O(1) though; and we hope to fix that for String at some point.

[bkamins]

I think skipmissing should be an allowed exception. The reason is that, although in this case the operation is not O(1), in it very often used in statistics and the fact that length does not work on it is a major usability issue reported by many users.

[nalimilan]

Contractually, length should probably be O(1) though; and we hope to fix that for String at some point.

Is that really possible? Note that we also define length(graphemes(::String)), which is unlikely to be made O(1).

Marking for triage to get a decision.

[JeffBezanson]

This feels weird to me but it's hard to point to a real problem it causes, so might as well?