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

better upper bounds in apply_type_tfunc for NamedTuple #32699

Closed
quinnj opened this issue Jul 26, 2019 · 8 comments · Fixed by #36366
Closed

better upper bounds in apply_type_tfunc for NamedTuple #32699

quinnj opened this issue Jul 26, 2019 · 8 comments · Fixed by #36366
Assignees
Labels
compiler:inference Type inference

Comments

@quinnj
Copy link
Member

quinnj commented Jul 26, 2019

Here's an example where the resulting NamedTuple{names, _A) where _A type is not ideal since it completely discards valid information up to that point.

8 ── %21 = Base.getfield(%14, :id)::Array{Int64,1}%22 = Base.arrayref(true, %21, %15)::Int64%23 = Base.getfield(%16, :salary)::Array{Union{Missing, Float64},1}%24 = Base.arrayref(true, %23, %17)::Union{Missing, Float64}%25 = (isa)(%24, Missing)::Bool
└───       goto #10 if not %25
9 ──       goto #13
10%28 = (isa)(%24, Float64)::Bool
└───       goto #12 if not %28
11%30 = π (%24, Float64)
│    %31 = Base.sitofp(Float64, 2)::Float64%32 = Base.mul_float(%30, %31)::Float64
└───       goto #13
12 ─       Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
└───       $(Expr(:unreachable))::Union{}
13%36 = φ (#9 => $(QuoteNode(missing)), #11 => %32)::Union{Missing, Float64}%37 = Core.tuple(%22, %36)::Tuple{Int64,Union{Missing, Float64}}%38 = (NamedTuple{(:id, :y),T} where T<:Tuple)(%37)::NamedTuple{(:id, :y),_A} where _A

@JeffBezanson thought it should be valid/fine to include an upper bound here, so that we end up with NamedTuple{(:id, :y), _A} where _A <: Tuple{Int64, Union{Missing, Float64}

@JeffBezanson JeffBezanson changed the title Include type parameter upper bounds better upper bounds in apply_type_tfunc for NamedTuple Jul 30, 2019
@JeffBezanson JeffBezanson added the compiler:inference Type inference label Jul 30, 2019
@bramtayl
Copy link
Contributor

bramtayl commented Aug 9, 2019

How can I help here? I know basically no C++ or Scheme but willing to learn. I'd really like to be able to use vanilla NamedTuples in LightQuery

@quinnj
Copy link
Member Author

quinnj commented Nov 2, 2019

Note that @JeffBezanson and I had a quick chat about this offline and #33326 currently takes priority to improve compiler times before we add a bit more work for it.

@bramtayl
Copy link
Contributor

@JeffBezanson sorry to bother you but looking for an update here. I was considering moving LightQuery over to using Base NamedTuples in hopes that some day performance issues will be resolved. Is that likely to happen at some point?

@bramtayl
Copy link
Contributor

bramtayl commented Jul 16, 2020

Yay!!! I've been playing around with this and it's working great. I'm running into problems with pairs, though? Which is inconvenient because keyword arguments get passed through pairs?

function test()
    pairs((a = if rand(Bool)
        1
    else
        "1"
    end,))
end
@code_warntype(test())
# Body::Base.Iterators.Pairs{Symbol,_A,Tuple{Symbol},_B} where _B where _A

@bramtayl
Copy link
Contributor

Nope, nevermind I don't think pairs the problem I hit. Instead, I'm having trouble merging in new (unstable) fields based on old (unstable) fields.

function test()
    data = (a = if rand(Bool)
        1
    else
        "1"
    end,)
    merge(data, (b = data.a,))
end
@code_warntype test() # NamedTuple{(:a, :b),_A} where _A<:Tuple

@bramtayl
Copy link
Contributor

And also, that propertynames for NamedTuples isn't pure. Given that they are encoded in the type for named tuples and part of the public interface, it seems like it might be a reasonable thing to do.

@bramtayl
Copy link
Contributor

Well anyways, the master version of LightQuery uses Base named tuples. It is a lot more convenient to use for sure. But benchmarking seems to show that performance goes out the window once you add a column with an unstable eltype.

@bramtayl
Copy link
Contributor

Should I open an issue for the merge problem?

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

Successfully merging a pull request may close this issue.

3 participants