Mirror readinto from the pyarrow stream onto ArrowFile - #2111
Open
hampsterx wants to merge 1 commit into
Open
Conversation
`ArrowFile` mirrors a fixed method list from the pyarrow stream it wraps and `readinto` is not on it, though `pyarrow.NativeFile` implements it. That leaves it the one file class here without one, so code written against the `AbstractBufferedFile` contract breaks on an Arrow-backed filesystem and nowhere else. `LibArchiveFileSystem` is the in-repo case, and it fails silently. Its `custom_reader` calls `readinto` from a ctypes callback, which swallows the `AttributeError`, so libarchive sees a zero-length read and `ls()` returns an empty archive instead of raising. Buffered reads at least raise: a sized `read`, `peek` and `read1` each fill the buffer through `readinto`, while an unsized `read()` falls back to `read(-1)` and is unaffected. Gzip is how it turned up. `fsspec.compression` registers isal's `IGzipFile` as the `gzip` codec when `isal` imports, and that reader decompresses through `readinto`, so reading a `.gz` file through an Arrow-backed filesystem fails in an environment that happens to carry the package and reads fine everywhere else.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ArrowFilemirrors a fixed method list from the pyarrow stream it wraps, andreadintois not onit, though
pyarrow.NativeFileimplements it. That makes it the one file class here without one:AbstractBufferedFileimplementsreadinto(fsspec/spec.py), so anything writtenagainst the usual fsspec handle contract breaks on an Arrow-backed filesystem and nowhere else.
LibArchiveFileSystemis the in-repo case, and it fails silently.custom_readerdocuments that"the
fileobject must support the standardreadintoand 'seek' methods" and calls it from actypes callback. An
AttributeErrorthere is swallowed by ctypes, libarchive sees a zero-lengthread, and
ls()returns an empty archive rather than raising:(The swallow itself is pre-existing and independent of this patch: any
readintofailure in thatcallback yields an empty archive rather than an error. This just removes the trigger.)
Buffered reads raise properly at least:
A whole-file
read()is fine, since that path goes toread()on the raw object. A sized read,peek()andread1()each fill the buffer throughreadinto.Gzip is how it turned up.
fsspec/compression.pyregisters isal'sIGzipFileas thegzipcodecwhen
isalimports and the stdlibGzipFileonly when it does not, and isal decompresses throughreadinto. So a.gzfile read through any Arrow-backed filesystem fails in an environment thatmerely carries the package, and reads fine everywhere else. A transitive
xopendependency broughtisal in on x86-64 and AArch64, and every
.gzread started raising.One entry on the mirror list, following #1154 (
seekable) and #1944 (size).Tests
test_readintocovers the handle directly on both the seekable and non-seekable paths;test_readinto_supports_a_buffered_readeris parametrized overread(3),peek(3)andread1(3).All eight cases fail on
masterwith theAttributeErrorabove and pass with the one-line change.Neither the gzip nor the libarchive case is asserted: isal is not guaranteed on CI and libarchive is
optional, and a combined libarchive + pyarrow fixture does not exist here. Both were reproduced by
hand against
masterand this branch. One note for anyone reproducing the buffered cases: thepure-Python
_pyio.BufferedReaderusesraw.readfor sized reads and so does not show the bug,only the C
io.BufferedReaderdoes.A downstream workaround, for anyone hitting this before a release: panodata/omniload#314 mixes a
_openoverride into its Arrow-backed filesystem classes to expose the stream's ownreadinto.