Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work around Julia's Base.Sort.MissingOptimization bugs #78

Merged
merged 4 commits into from
Jun 15, 2023

Conversation

LilithHafner
Copy link
Member

This inserts a shim that fixes the bug reported in JuliaData/DataFrames.jl#3340. That bug was introduced by bugs in Julia 1.9, and should be fixed in 1.9.2 and 1.10.0 by JuliaLang/julia#50171. See JuliaLang/julia#50171 for a more detailed description of the Julia bugs.

These bugs should and will be fixed upstream, but in the interem, before those fixes make it into official Julia binaries, this PR shields users from those bugs.

The shim is scoped to the versions where the bug is present, which is a pretty narrow scope (but as of the time of this PR, that scope includes the latest stable version of Jula).

The first bug is in Julia's implementation getindex(::Base.Sort.WithoutMissingVector, ::UnitRange). This shim uses pretty severe type piracy, but I think that's okay because 1) it redefines a method that previously always errored and 2) it will disappear automatically in later versions of Julia.

The second bug is in Julia's handling of sort!(v::AbstractVector, lo::Integer, hi::Integer, alg::Algorithm, o::Ordering) when alg includes MissingOptimization, v has an eltype that is a union with Missing, lo:hi ≠ eachindex(v) and o isa Perm. Pretty niche, but it comes up, so we fix it. The fix here is to cut out MissingOptimization from the callsite that can have lo:hi ≠ eachindex(v). In theory, this patch could be restricted to the case when o isa Perm as well, but I'd rather keep it simple than save that teensy bit of performance in that special case. Of note, is that any performance regression caused by this will be merely reverting a performance gain introduced with MissingOptimization in Julia 1.9.

Also, some whitespace fixups that my editor did automatically.

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

Merging #78 (6faa1a0) into master (4f1b96e) will decrease coverage by 6.86%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   96.02%   89.16%   -6.86%     
==========================================
  Files           1        1              
  Lines         352      360       +8     
==========================================
- Hits          338      321      -17     
- Misses         14       39      +25     
Impacted Files Coverage Δ
src/SortingAlgorithms.jl 89.16% <100.00%> (-6.86%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@LilithHafner
Copy link
Member Author

This isn't done yet:

julia> v = rand(1000);

julia> sort!(collect(eachindex(v)), SortingAlgorithms.TimSort, Base.Order.Perm(Base.Order.Forward, v))
ERROR: ArgumentError: Base.Sort._sort!(::Vector{Int64}, ::SortingAlgorithms.TimSortAlg, ::Base.Order.Perm{Base.Order.ReverseOrdering{Base.Order.ForwardOrdering}, Base.ReinterpretArray{UInt64, 1, Float64, Vector{Float64}, false}}) is not defined

@LilithHafner
Copy link
Member Author

This third error is a false positive in Base's infinite dispatch loop detection. Fixed upstream by using a better dispatch loop detector and fixed in this shim by inserting explicit conversion from the old calling convention to the new calling convention.

@LilithHafner
Copy link
Member Author

Nightly tests are expected to fail until JuliaLang/julia#50171 merges because the shim is automatically removed in 1.10 and tests are now stricter than they were before.

@LilithHafner
Copy link
Member Author

I'm going to yolo this because it's a bug fix and releases are cheap. @oscardssmith, I'd still appreciate your review and I'm happy to release a follow-up patch if there's more to be done or if (hopefully not the case) I broke something else in this PR—tests are weak in this repo.

@LilithHafner LilithHafner merged commit ff693e2 into master Jun 15, 2023
@LilithHafner LilithHafner deleted the bugfix-MissingOptimization branch June 15, 2023 02:07
@oscardssmith
Copy link
Member

Looks good to me (that said, this is my first time looking at this code)

@LilithHafner
Copy link
Member Author

Okay, thanks, I'll go ahead with registering the release, then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants