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.

Sign up for GitHub

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

fix Vector{UInt8} writing #419

Closed
wants to merge 7 commits into from
Closed

fix Vector{UInt8} writing #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
224d94e
fix Vector{UInt8} writing
Moelf Apr 7, 2023
d5a1f12
remove debug
Moelf Apr 7, 2023
2d84241
more robust
Moelf Apr 7, 2023
7a22d95
more robust
Moelf Apr 8, 2023
3e0fff3
restore tests
Moelf Apr 8, 2023
a1e123b
Merge branch 'main' of https://github.com/apache/arrow-julia into uin…
Moelf Apr 13, 2023
18cee30
fix tests and give up fixedsizelist for now
Moelf Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ArrowTypes/src/ArrowTypes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ struct ListKind{stringtype} <: ArrowKind end
ListKind() = ListKind{false}()
isstringtype(::ListKind{stringtype}) where {stringtype} = stringtype
isstringtype(::Type{ListKind{stringtype}}) where {stringtype} = stringtype
isstringtype(::Type{T}) where {T} = nonmissingtype(T) <: Union{Base.CodeUnits, AbstractString}

ArrowKind(::Type{<:AbstractString}) = ListKind{true}()

Expand Down
7 changes: 4 additions & 3 deletions src/arraytypes/list.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,10 @@ function arrowvector(::ListKind, x, i, nl, fi, de, ded, meta; largelists::Bool=f
validity = ValidityBitmap(x)
flat = ToList(x; largelists=largelists)
offsets = Offsets(UInt8[], flat.inds)
if eltype(flat) == UInt8 # binary or utf8string
OT = origtype(flat)
if eltype(flat) == UInt8 && ArrowTypes.isstringtype(OT)# utf8 or byte string
data = flat
T = origtype(flat)
T = OT
else
data = arrowvector(flat, i, nl + 1, fi, de, ded, nothing; lareglists=largelists, kw...)
T = withmissing(eltype(x), Vector{eltype(data)})
Expand All @@ -208,7 +209,7 @@ function compress(Z::Meta.CompressionType, comp, x::List{T, O, A}) where {T, O,
offsets = compress(Z, comp, x.offsets.offsets)
buffers = [validity, offsets]
children = Compressed[]
if eltype(A) == UInt8
if eltype(A) == UInt8 && ArrowTypes.isstringtype(T)# utf8 or byte string
push!(buffers, compress(Z, comp, x.data))
else
push!(children, compress(Z, comp, x.data))
Expand Down
5 changes: 3 additions & 2 deletions src/arraytypes/map.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ function makenodesbuffers!(col::Union{Map{T, O, A}, List{T, O, A}}, fieldnodes,
push!(fieldbuffers, Buffer(bufferoffset, blen))
@debugv 1 "made field buffer: bufferidx = $(length(fieldbuffers)), offset = $(fieldbuffers[end].offset), len = $(fieldbuffers[end].length), padded = $(padding(fieldbuffers[end].length, alignment))"
bufferoffset += padding(blen, alignment)
if eltype(A) == UInt8
if eltype(A) == UInt8 && ArrowTypes.isstringtype(T)
blen = length(col.data)
push!(fieldbuffers, Buffer(bufferoffset, blen))
@debugv 1 "made field buffer: bufferidx = $(length(fieldbuffers)), offset = $(fieldbuffers[end].offset), len = $(fieldbuffers[end].length), padded = $(padding(fieldbuffers[end].length, alignment))"
Expand All @@ -110,7 +110,8 @@ function writebuffer(io, col::Union{Map{T, O, A}, List{T, O, A}}, alignment) whe
@debugv 1 "writing array: col = $(typeof(col.offsets.offsets)), n = $n, padded = $(padding(n, alignment))"
writezeros(io, paddinglength(n, alignment))
# write values array
if eltype(A) == UInt8
#
if eltype(A) == UInt8 && ArrowTypes.isstringtype(T)
n = writearray(io, UInt8, col.data)
@debugv 1 "writing array: col = $(typeof(col.data)), n = $n, padded = $(padding(n, alignment))"
writezeros(io, paddinglength(n, alignment))
Expand Down
32 changes: 15 additions & 17 deletions src/eltypes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,23 +393,21 @@ end

# arrowtype will call fieldoffset recursively for children
function arrowtype(b, x::List{T, O, A}) where {T, O, A}
if eltype(A) == UInt8
if T <: AbstractString || T <: Union{AbstractString, Missing}
if O == Int32
Meta.utf8Start(b)
return Meta.Utf8, Meta.utf8End(b), nothing
else # if O == Int64
Meta.largUtf8Start(b)
return Meta.LargeUtf8, Meta.largUtf8End(b), nothing
end
else # if Vector{UInt8}
if O == Int32
Meta.binaryStart(b)
return Meta.Binary, Meta.binaryEnd(b), nothing
else # if O == Int64
Meta.largeBinaryStart(b)
return Meta.LargeBinary, Meta.largeBinaryEnd(b), nothing
end
if eltype(A) == UInt8 && T <: AbstractString || T <: Union{AbstractString, Missing}
if O == Int32
Meta.utf8Start(b)
return Meta.Utf8, Meta.utf8End(b), nothing
else # if O == Int64
Meta.largUtf8Start(b)
return Meta.LargeUtf8, Meta.largUtf8End(b), nothing
end
elseif eltype(A) == UInt8 && ArrowTypes.isstringtype(T)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only additional type cathed here is Base.CodeUnits.

Ideally, we want the first (above) used for long string and this one for short-string, but that's not a well defined concept, so for now we just say, "use CodeUnits if you want output byte-string in Arrow"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "cathed" mean?

Copy link
Contributor Author

@Moelf Moelf May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, "caught" (I probably tried to type catched, isn't a word), as in caught by the condition

if O == Int32
Meta.binaryStart(b)
return Meta.Binary, Meta.binaryEnd(b), nothing
else # if O == Int64
Meta.largeBinaryStart(b)
return Meta.LargeBinary, Meta.largeBinaryEnd(b), nothing
end
else
children = [fieldoffset(b, "", x.data)]
Expand Down
6 changes: 5 additions & 1 deletion test/testtables.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,11 @@ function testtable(nm, t, writekw, readkw, extratests)
@test length(tt) == length(t)
@test all(isequal.(values(t), values(tt)))
# compressed
io = Arrow.tobuffer(t; compress=((:lz4, :zstd)[rand(1:2)]), writekw...)
io = Arrow.tobuffer(t; compress=:lz4, writekw...)
tt = Arrow.Table(io; readkw...)
@test length(tt) == length(t)
@test all(isequal.(values(t), values(tt)))
io = Arrow.tobuffer(t; compress=:zstd, writekw...)
tt = Arrow.Table(io; readkw...)
@test length(tt) == length(t)
@test all(isequal.(values(t), values(tt)))
Expand Down