Skip to content

Commit

Permalink
move StructElement definition to ArrowTypes and add documentation
Browse files Browse the repository at this point in the history
  • Loading branch information
jrevels committed Dec 4, 2023
1 parent 1244f94 commit be7a4fe
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 35 deletions.
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
name = "Arrow"
uuid = "69666777-d1a9-59fb-9406-91d4454c9d45"
authors = ["quinnj <quinn.jacobd@gmail.com>"]
version = "2.6.3"
version = "2.7.0"

[deps]
ArrowTypes = "31f734f8-188a-4ce0-8406-c8a06bd891cd"
Expand Down
2 changes: 1 addition & 1 deletion src/ArrowTypes/Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
name = "ArrowTypes"
uuid = "31f734f8-188a-4ce0-8406-c8a06bd891cd"
authors = ["quinnj <quinn.jacobd@gmail.com>"]
version = "2.2.2"
version = "2.3"

[deps]
Sockets = "6462fe0b-24de-5631-8697-dd941f90decc"
Expand Down
16 changes: 15 additions & 1 deletion src/ArrowTypes/src/ArrowTypes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,15 @@ overload is necessary.
A few `ArrowKind`s have/allow slightly more custom overloads for their `fromarrow` methods:
* `ListKind{true}`: for `String` types, they may overload `fromarrow(::Type{T}, ptr::Ptr{UInt8}, len::Int) = ...` to avoid
materializing a `String`
* `StructKind`: must overload `fromarrow(::Type{T}, x...)` where individual fields are passed as separate
* `StructKind`:
* May overload `fromarrow(::Type{T}, x...)` where individual fields are passed as separate
positional arguments; so if my custom type `Interval` has two fields `first` and `last`, then I'd overload like
`ArrowTypes.fromarrow(::Type{Interval}, first, last) = ...`. Note the default implementation is
`ArrowTypes.fromarrow(::Type{T}, x...) = T(x...)`, so if your type already accepts all arguments in a constructor
no additional `fromarrow` method should be necessary (default struct constructors have this behavior).
* Alternatively, may overload `fromarrow(::Type{T}, x::ArrowTypes.StructElement)`, where fields are stored in
`x.fields::NamedTuple`. Use this approach over the positional approach when you need to implement deserialization
in a manner that is agnostic to the field order used by the serializer.
"""
function fromarrow end
fromarrow(::Type{T}, x::T) where {T} = x
Expand Down Expand Up @@ -302,6 +306,16 @@ struct StructKind <: ArrowKind end

ArrowKind(::Type{<:NamedTuple}) = StructKind()

struct StructElement{T<:NamedTuple}
fields::T
end

function fromarrow(::Type{T}, x::StructElement) where {T}
return fromarrow(T, values(x.fields)...)
end

fromarrow(::Type{Union{Missing,T}}, x::StructElement) where {T} = fromarrow(T, x) # resolves method ambiguity

fromarrow(
::Type{NamedTuple{names,types}},
x::NamedTuple{names,types},
Expand Down
23 changes: 12 additions & 11 deletions src/arraytypes/struct.jl
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.

struct StructElement{T<:NamedTuple}
fields::T
end

function ArrowTypes.fromarrow(::Type{T}, x::StructElement) where {T}
return fromarrow(T, values(x.fields)...)
end

ArrowTypes.fromarrow(::Type{Union{Missing,T}}, x::StructElement) where {T} = fromarrow(T, x) # resolves method ambiguity

"""
Arrow.Struct
Expand All @@ -43,6 +33,17 @@ isnamedtuple(T) = false
istuple(::Type{<:Tuple}) = true
istuple(T) = false

if isdefined(ArrowTypes, :StructElement)
# https://github.com/apache/arrow-julia/pull/493
@inline function _struct_access_fromarrow(::Type{Val{fnames}}, T::Type, vals) where {fnames}
return ArrowTypes.fromarrow(T, ArrowTypes.StructElement(NamedTuple{fnames}(vals)))
end
else
@inline function _struct_access_fromarrow(::Type, T::Type, vals)
return ArrowTypes.fromarrow(T, vals...)
end
end

@propagate_inbounds function Base.getindex(
s::Struct{T,S,fnames},
i::Integer,
Expand All @@ -54,7 +55,7 @@ istuple(T) = false
if isnamedtuple(NT) || istuple(NT)
return NT(vals)
else
return ArrowTypes.fromarrow(NT, StructElement(NamedTuple{fnames}(vals)))
return _struct_access_fromarrow(Val{fnames}, NT, vals)
end
end

Expand Down
42 changes: 21 additions & 21 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1014,28 +1014,28 @@ end
# @test isequal(table.v, table2.v)

# end

@testset "# 493" begin
# This test stresses the existence of the mechanism
# implemented in https://github.com/apache/arrow-julia/pull/493,
# but doesn't stress the actual use case that motivates
# that mechanism, simply because it'd be more annoying to
# write that test; see the PR for details.
struct Foo493
x::Int
y::Int
end
Arrow.ArrowTypes.arrowname(::Type{Foo493}) = Symbol("JuliaLang.Foo493")
Arrow.ArrowTypes.JuliaType(::Val{Symbol("JuliaLang.Foo493")}, T) = Foo493
function Arrow.ArrowTypes.fromarrow(::Type{Foo493}, f::Arrow.StructElement)
return Foo493(f.fields.x + 1, f.fields.y + 1)
if isdefined(ArrowTypes, :StructElement)
@testset "# 493" begin
# This test stresses the existence of the mechanism
# implemented in https://github.com/apache/arrow-julia/pull/493,
# but doesn't stress the actual use case that motivates
# that mechanism, simply because it'd be more annoying to
# write that test; see the PR for details.
struct Foo493
x::Int
y::Int
end
ArrowTypes.arrowname(::Type{Foo493}) = Symbol("JuliaLang.Foo493")
ArrowTypes.JuliaType(::Val{Symbol("JuliaLang.Foo493")}, T) = Foo493
function ArrowTypes.fromarrow(::Type{Foo493}, f::ArrowTypes.StructElement)
return Foo493(f.fields.x + 1, f.fields.y + 1)
end
t = (; f=[Foo493(1, 2), Foo493(3, 4)])
buf = Arrow.tobuffer(t)
tbl = Arrow.Table(buf)
@test tbl.f[1] === Foo493(2, 3)
@test tbl.f[2] === Foo493(4, 5)
end
t = (; f=[Foo493(1, 2), Foo493(3, 4)])
buf = Arrow.tobuffer(t)
tbl = Arrow.Table(buf)
@test tbl.f[1] === Foo493(2, 3)
@test tbl.f[2] === Foo493(4, 5)
end

end # @testset "misc"
end

0 comments on commit be7a4fe

Please sign in to comment.