Investigate failure

[quinnj]

as reported by @domluna on slack: https://gist.github.com/domluna/409cda49cba588300bb3d51e6a5c8543

[garborg]

I hit the same issue with an optional field (MWE pasted below, but originally hit the issue with Vector{[Expanded]Bar} round-tripped through Arrow.jl):

julia> using Strapping

julia> using StructTypes

julia> struct Foo
           a::Float64
           b::Float64
       end

julia> StructTypes.StructType(::Type{Foo}) = StructTypes.Struct()

julia> struct Bar
           main::Foo
           other::Union{Nothing,Foo}
       end

julia> StructTypes.StructType(::Type{Bar}) = StructTypes.Struct()

julia> b = Bar(Foo(1,2), Foo(3, 4))
Bar(Foo(1.0, 2.0), Foo(3.0, 4.0))

julia> de_b = Strapping.deconstruct(b)
Strapping.DeconstructedRowsIterator{Bar, Vector{Bar}}(Bar[Bar(Foo(1.0, 2.0), Foo(3.0, 4.0))], [1], 1, [:main_a, :main_b, :other_a, :other_b], Type[Float64, Float64, Float64, Float64], Strapping.FieldNode[Strapping.FieldNode(1, :main, Strapping.FieldNode(1, :a, nothing)), Strapping.FieldNode(1, :main, Strapping.FieldNode(2, :b, nothing)), Strapping.FieldNode(2, :other, Strapping.FieldNode(1, :a, nothing)), Strapping.FieldNode(2, :other, Strapping.FieldNode(2, :b, nothing))], Dict(:main_a => 1, :main_b => 2, :other_b => 4, :other_a => 3))

julia> re_b = Strapping.construct(Bar, de_b)
ERROR: ArgumentError: type does not have a definite number of fields
Stacktrace:
  [1] fieldcount(t::Any)
    @ Base ./reflection.jl:707
  [2] construct
    @ ~/.julia/packages/StructTypes/dN2cf/src/StructTypes.jl:573 [inlined]
  [3] construct(::StructTypes.UnorderedStruct, ::Type{Union{Nothing, Foo}}, row::Strapping.DeconstructedRow{Bar}, prefix::Symbol, offset::Base.RefValue{Int64}; kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:149
  [4] construct(::StructTypes.UnorderedStruct, ::Type{Union{Nothing, Foo}}, row::Strapping.DeconstructedRow{Bar}, prefix::Symbol, offset::Base.RefValue{Int64})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:149
  [5] construct(::StructTypes.UnorderedStruct, ::Type{Union{Nothing, Foo}}, row::Strapping.DeconstructedRow{Bar}, prefix::Symbol, offset::Base.RefValue{Int64}, i::Int64, nm::Symbol; kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:158
  [6] construct(::StructTypes.UnorderedStruct, ::Type{Union{Nothing, Foo}}, row::Strapping.DeconstructedRow{Bar}, prefix::Symbol, offset::Base.RefValue{Int64}, i::Int64, nm::Symbol)
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:158
  [7] (::Strapping.var"#6#7"{Bar, Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, Strapping.DeconstructedRow{Bar}, Symbol, Base.RefValue{Int64}})(i::Int64, nm::Symbol, TT::Type)
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:150
  [8] construct(f::Strapping.var"#6#7"{Bar, Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, Strapping.DeconstructedRow{Bar}, Symbol, Base.RefValue{Int64}}, #unused#::Type{Bar})
    @ StructTypes ~/.julia/packages/StructTypes/dN2cf/src/StructTypes.jl:584
  [9] construct(::StructTypes.UnorderedStruct, ::Type{Bar}, row::Strapping.DeconstructedRow{Bar}, prefix::Symbol, offset::Base.RefValue{Int64}; kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:149
 [10] construct(::StructTypes.UnorderedStruct, ::Type{Bar}, row::Strapping.DeconstructedRow{Bar}, prefix::Symbol, offset::Base.RefValue{Int64}) (repeats 2 times)
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:149
 [11] construct(::Type{Bar}, rows::Strapping.DeconstructedRowsIterator{Bar, Vector{Bar}}, row::Strapping.DeconstructedRow{Bar}, st::Tuple{Int64, Int64, Int64}; kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:118
 [12] construct(::Type{Bar}, rows::Strapping.DeconstructedRowsIterator{Bar, Vector{Bar}}, row::Strapping.DeconstructedRow{Bar}, st::Tuple{Int64, Int64, Int64})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:118
 [13] construct(::Type{Bar}, source::Strapping.DeconstructedRowsIterator{Bar, Vector{Bar}}; silencewarnings::Bool, kw::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:99
 [14] construct(::Type{Bar}, source::Strapping.DeconstructedRowsIterator{Bar, Vector{Bar}})
    @ Strapping ~/.julia/packages/Strapping/uLJ7i/src/Strapping.jl:95
 [15] top-level scope
    @ REPL[77]:1

[garborg]

Perhaps name this issue something like "Issue constructing optional structs"

[quinnj]

Ok, sorry for the delay in responding here: @garborg , @chris-b1, and @Sov-trotter. You've all reported similar issues with regards to how Union types are handled in Strapping.jl.

While digging into things, this is a bit tricky. If the Union type is a Union of scalars, like Union{Int, Nothing}, then I think it's actually pretty straightforward, because we have a pretty generic "get any-typed scalar" routine we can drop down to and it doesn't matter if it's nothing or 10, it will just work.

But in the case @garborg mentions above, with Union{Nothing, Foo}, it gets more complicated, because for Struct/Mutable, we rely on having a fixed # of fields we're indexing over, and if we get nested structs, that becomes even more critical.

I think I need to take a step back from the current implementation and see if we can redesign things to better account for optional fields whether they're scalar or aggregates. We also need to enhance the UnorderedStruct case, as mentioned in #19, since it's currently following the behavior of OrderedStruct instead.

[quinnj]

Ok, so here's some more after thinking further. I think the construct case for Union{T, Nothing} where T is a Struct really depends on what we do in the deconstruct case. i.e. we can start with Bar(Foo(1, 2), nothing), but how does that get output as a 2D table. One option, again assuming the Foo/Bar definitions in @garborg's post above, with an instance like b = Bar(Foo(1, 2), nothing), is we could output:

(main_a=1, main_b=2, other=nothing)

alternatively, we could output:

(main_a=1, main_b=2, other_a=nothing, other_b=nothing)

but the latter would require probably special-casing Union{Nothing, T} where T is a Struct.

For the record, we currently output:

(main_a=1, main_b=2)

so the other field is just ignored 👎 .

The next case that is tricky is when we consider multiple Bar instances where we need to output a consistent schema. i.e. if we have x = [Bar(Foo(1, 2), Foo(3, 4)), Bar(Foo(5, 6), nothing)]. It seems like we'd have to output:

[
    (main_a=1, main_b=2, other_a=3, other_b=4),
    (main_a=5, main_b=6, other_a=nothing, other_b=nothing)
]

The problem with outputting that representation is the following: consider if the definition of Foo was instead:

struct Foo
    a::Union{Nothing, Float64}
    b::Union{Nothing, Float64}
end

Now our row like (main_a=5, main_b=6, other_a=nothing, other_b=nothing) is ambiguous. Should we return Bar(..., nothing), or Bar(..., Foo(nothing, nothing))? Oof.