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

Avoid assuming field values can be used in constructors #135

Closed
omus opened this issue Feb 23, 2021 · 1 comment
Closed

Avoid assuming field values can be used in constructors #135

omus opened this issue Feb 23, 2021 · 1 comment

Comments

@omus
Copy link
Contributor

omus commented Feb 23, 2021

julia> using Arrow, Intervals

julia> table = (col = [
           Interval{Closed,Unbounded}(1,nothing),
       ],)
(col = Interval{Int64,Closed,Unbounded}[Interval{Int64,Closed,Unbounded}(1, nothing)],)

julia> table.col
1-element Array{Interval{Int64,Closed,Unbounded},1}:
 Interval{Int64,Closed,Unbounded}(1, nothing)

julia> Arrow.write("un.arrow", table);

julia> Arrow.Table("un.arrow").col
1-element Arrow.Struct{Interval{Int64,Closed,Unbounded},Tuple{Arrow.Primitive{Int64,Array{Int64,1}},Arrow.Primitive{Int64,Array{Int64,1}}}}:
Error showing value of type Arrow.Struct{Interval{Int64,Closed,Unbounded},Tuple{Arrow.Primitive{Int64,Array{Int64,1}},Arrow.Primitive{Int64,Array{Int64,1}}}}:
ERROR: MethodError: no method matching Interval{Int64,Closed,Unbounded}(::Int64, ::Int64)
Closest candidates are:
  Interval{Int64,Closed,Unbounded}(::T, ::T, ::Inclusivity) where {T, L, R} at /Users/omus/.julia/dev/Intervals/src/deprecated.jl:34
  Interval{Int64,Closed,Unbounded}(::Any, ::Any, ::Inclusivity) where {T, L, R} at /Users/omus/.julia/dev/Intervals/src/deprecated.jl:44
  Interval{Int64,Closed,Unbounded}(::T, ::Nothing) where {T, L<:Intervals.Bounded, R<:Unbounded} at /Users/omus/.julia/dev/Intervals/src/interval.jl:87
  ...

The core of the problem here is that Interval{T,L,R} contains two fields :first and :last which must both be of type T. When constructing an unbounded interval we use nothing to represent unbounded in the constructor but internally just store an arbitrary value of T for the unbounded endpoint. I'll note that native Julia serialization doesn't encounter this problem.

One workaround would be to have Arrow.jl provided callback when constructing the Julia type. That way we could revise the constructor arguments based upon the parametric types.

@quinnj
Copy link
Member

quinnj commented Mar 25, 2021

Ok, with the functionality in #156, we can now support this like:

using Arrow, Intervals
table = (col = [
    Interval{Closed,Unbounded}(1,nothing),
],)
const NAME = Symbol("JuliaLang.Interval")
ArrowTypes.arrowname(::Type{Interval{T, L, R}}) where {T, L, R} = NAME
const LOOKUP = Dict(
    "Closed" => Closed,
    "Unbounded" => Unbounded
)
ArrowTypes.arrowmetadata(::Type{Interval{T, L, R}}) where {T, L, R} = string(L, ".", R)
function ArrowTypes.JuliaType(::Val{NAME}, ::Type{NamedTuple{names, types}}, meta) where {names, types}
    L, R = split(meta, ".")
    return Interval{fieldtype(types, 1), LOOKUP[L], LOOKUP[R]}
end
ArrowTypes.fromarrow(::Type{Interval{T, L, R}}, first, last) where {T, L, R} = Interval{L, R}(first, R == Unbounded ? nothing : last)
io = Arrow.tobuffer(table)
tbl = Arrow.Table(io)

I don't quite know Intervals well enough to know if that's general enough to work, but happy to iterate on the design if not.

@quinnj quinnj closed this as completed Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants