Copilot commented on code in PR #616:
URL: https://github.com/apache/arrow-julia/pull/616#discussion_r4488793169
##########
test/runtests.jl:
##########
@@ -1107,6 +1107,19 @@ end
joinpath(dirname(pathof(Arrow)),
"../test/reject_reason_trimmed.arrow"),
)
@test t.reject_reason[end] == "POST_ONLY"
+
+ # Compressed reinterp for UInt8 returns GC-owned decompressed
Vector{UInt8}
+ raw = collect(codeunits("Luxembourg_120203202_18815"))
+ compressed = vcat(
+ reinterpret(UInt8, [Int64(length(raw))]),
+ transcode(Arrow.CodecZstd.ZstdCompressor(), raw),
Review Comment:
The codec-instance overload of `transcode` requires an initialized codec,
but this freshly constructed `ZstdCompressor()` has not been initialized. The
existing custom-compressor test explicitly initializes it at lines 275–276. As
written, this test fails during compression before exercising the GC fix. Pass
the codec type instead so the convenience overload handles initialization and
finalization.
##########
src/table.jl:
##########
@@ -912,7 +913,7 @@ function build(
nodeidx += 1
meta = buildmetadata(f.custom_metadata)
T = juliaeltype(f, meta, convert)
- return View{T}(batch.bytes, validity, views, inline, buffers, len, meta),
+ return View{T}(bytes, validity, views, inline, buffers, len, meta),
Review Comment:
The new regression test exercises only `reinterp(UInt8, ...)`, so it does
not protect this separate fix to the owner of `View.data`. Reverting this line
to `batch.bytes` would leave that test unchanged. Please add a compressed
Utf8View/BinaryView regression that retains only the constructed column, forces
GC after construction, and checks both inline and non-inline values, so the
view-descriptor buffer's lifetime is covered as well.
##########
test/runtests.jl:
##########
@@ -1107,6 +1107,19 @@ end
joinpath(dirname(pathof(Arrow)),
"../test/reject_reason_trimmed.arrow"),
)
@test t.reject_reason[end] == "POST_ONLY"
+
+ # Compressed reinterp for UInt8 returns GC-owned decompressed
Vector{UInt8}
+ raw = collect(codeunits("Luxembourg_120203202_18815"))
+ compressed = vcat(
+ reinterpret(UInt8, [Int64(length(raw))]),
+ transcode(Arrow.CodecZstd.ZstdCompressor(), raw),
+ )
+ batch = (; bytes=compressed, pos=1)
+ buf = (; offset=0, length=length(compressed))
+ comp = (; codec=Arrow.Meta.CompressionType.ZSTD)
+ _, A = Arrow.reinterp(UInt8, batch, buf, comp)
Review Comment:
The equality check after `GC.gc()` does not reliably detect the original
ownership bug: reclaimed storage can retain its previous contents, so a
dangling `unsafe_wrap` array may still compare equal to `raw`. Capture both
return values and assert that they are the same array to make this a
deterministic regression test for returning the owning decompressed vector.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
