Skip to content

REGR: memory_map with non-UTF8 encoding #40994

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

Merged
merged 1 commit into from
Apr 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.2.5.rst
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ including other versions of pandas.
Fixed regressions
~~~~~~~~~~~~~~~~~
- Regression in :func:`concat` between two :class:`DataFrames` where one has an :class:`Index` that is all-None and the other is :class:`DatetimeIndex` incorrectly raising (:issue:`40841`)
-
- Regression in :func:`read_csv` when using ``memory_map=True`` with an non-UTF8 encoding (:issue:`40986`)
-

.. ---------------------------------------------------------------------------
Expand Down
41 changes: 36 additions & 5 deletions pandas/io/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,12 @@ def get_handle(

# memory mapping needs to be the first step
handle, memory_map, handles = _maybe_memory_map(
handle, memory_map, ioargs.encoding, ioargs.mode, errors
handle,
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't object to passing by kwargs for easier reading

memory_map,
ioargs.encoding,
ioargs.mode,
errors,
ioargs.compression["method"] not in _compression_to_extension,
)

is_path = isinstance(handle, str)
Expand Down Expand Up @@ -820,7 +825,18 @@ class _MMapWrapper(abc.Iterator):

"""

def __init__(self, f: IO):
def __init__(
self,
f: IO,
encoding: str = "utf-8",
errors: str = "strict",
decode: bool = True,
):
self.encoding = encoding
self.errors = errors
self.decoder = codecs.getincrementaldecoder(encoding)(errors=errors)
self.decode = decode

self.attributes = {}
for attribute in ("seekable", "readable", "writeable"):
if not hasattr(f, attribute):
Expand All @@ -836,19 +852,30 @@ def __getattr__(self, name: str):
def __iter__(self) -> _MMapWrapper:
return self

def read(self, size: int = -1) -> str | bytes:
Copy link
Member Author

Choose a reason for hiding this comment

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

This function could be removed if the c-engine handled non-utf-8 better.

The PR in its current form will in case of the c-engine: 1) use mmap to read the entire file 2) decode it appropriately (this function) 3) the c-code will encode the now utf-8 string into bytes again. It would be more efficient if the c-engine supported non-utf-8 in more places. I will look into that, but that might take some time.

Copy link
Contributor

Choose a reason for hiding this comment

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

prob not worth the effort to handle non-utf-8 better, but if you want to look...ok

# CSV c-engine uses read instead of iterating
content: bytes = self.mmap.read(size)
if self.decode:
# memory mapping is applied before compression. Encoding should
# be applied to the de-compressed data.
return content.decode(self.encoding, errors=self.errors)
return content

def __next__(self) -> str:
newbytes = self.mmap.readline()

# readline returns bytes, not str, but Python's CSV reader
# expects str, so convert the output to str before continuing
newline = newbytes.decode("utf-8")
newline = self.decoder.decode(newbytes)

# mmap doesn't raise if reading past the allocated
# data but instead returns an empty string, so raise
# if that is returned
if newline == "":
raise StopIteration
return newline

# IncrementalDecoder seems to push newline to the next line
return newline.lstrip("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

It may leave \r at the end of the line when newline is CRLF, which is often used in windows.

Maybe try newline.lstrip("\n").rstrip("\r")?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! It might be worth adding tests to directly test the output of the mmap wrapper independent of read_csv.

I would assume that CRLF is covered by some of the Windows CI but it might be that the c-engine and python's csv are robust enough to ignore that :)



def _maybe_memory_map(
Expand All @@ -857,6 +884,7 @@ def _maybe_memory_map(
encoding: str,
mode: str,
errors: str | None,
decode: bool,
) -> tuple[FileOrBuffer, bool, list[Buffer]]:
"""Try to memory map file/buffer."""
handles: list[Buffer] = []
Expand All @@ -877,7 +905,10 @@ def _maybe_memory_map(
try:
# error: Argument 1 to "_MMapWrapper" has incompatible type "Union[IO[Any],
# RawIOBase, BufferedIOBase, TextIOBase, mmap]"; expected "IO[Any]"
wrapped = cast(mmap.mmap, _MMapWrapper(handle)) # type: ignore[arg-type]
wrapped = cast(
mmap.mmap,
_MMapWrapper(handle, encoding, errors, decode), # type: ignore[arg-type]
)
handle.close()
handles.remove(handle)
handles.append(wrapped)
Expand Down
19 changes: 0 additions & 19 deletions pandas/io/parsers/c_parser_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,6 @@ def __init__(self, src: FilePathOrBuffer, **kwds):
assert self.handles is not None
for key in ("storage_options", "encoding", "memory_map", "compression"):
kwds.pop(key, None)
if self.handles.is_mmap and hasattr(self.handles.handle, "mmap"):
# error: Item "IO[Any]" of "Union[IO[Any], RawIOBase, BufferedIOBase,
# TextIOBase, TextIOWrapper, mmap]" has no attribute "mmap"

# error: Item "RawIOBase" of "Union[IO[Any], RawIOBase, BufferedIOBase,
# TextIOBase, TextIOWrapper, mmap]" has no attribute "mmap"

# error: Item "BufferedIOBase" of "Union[IO[Any], RawIOBase, BufferedIOBase,
# TextIOBase, TextIOWrapper, mmap]" has no attribute "mmap"

# error: Item "TextIOBase" of "Union[IO[Any], RawIOBase, BufferedIOBase,
# TextIOBase, TextIOWrapper, mmap]" has no attribute "mmap"

# error: Item "TextIOWrapper" of "Union[IO[Any], RawIOBase, BufferedIOBase,
# TextIOBase, TextIOWrapper, mmap]" has no attribute "mmap"

# error: Item "mmap" of "Union[IO[Any], RawIOBase, BufferedIOBase,
# TextIOBase, TextIOWrapper, mmap]" has no attribute "mmap"
self.handles.handle = self.handles.handle.mmap # type: ignore[union-attr]

try:
self._reader = parsers.TextReader(self.handles.handle, **kwds)
Expand Down
17 changes: 17 additions & 0 deletions pandas/tests/io/parser/test_encoding.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,3 +220,20 @@ def test_parse_encoded_special_characters(encoding):

expected = DataFrame(data=[[":foo", 0], ["bar", 1], ["baz", 2]], columns=["a", "b"])
tm.assert_frame_equal(result, expected)


@pytest.mark.parametrize("encoding", ["utf-8", None, "utf-16", "cp1255", "latin-1"])
def test_encoding_memory_map(all_parsers, encoding):
# GH40986
parser = all_parsers
expected = DataFrame(
{
"name": ["Raphael", "Donatello", "Miguel Angel", "Leonardo"],
"mask": ["red", "purple", "orange", "blue"],
"weapon": ["sai", "bo staff", "nunchunk", "katana"],
}
)
with tm.ensure_clean() as file:
expected.to_csv(file, index=False, encoding=encoding)
df = parser.read_csv(file, encoding=encoding, memory_map=True)
tm.assert_frame_equal(df, expected)