Skip to content

BUG: Add extra check for failing UTF-8 conversion #32548

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

Merged
merged 5 commits into from
Mar 12, 2020
Merged

BUG: Add extra check for failing UTF-8 conversion #32548

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6bb3f9
Add extra check for failing UTF-8 conversion
roberthdevries Mar 8, 2020
a53e2f0
Reformatted by black
roberthdevries Mar 8, 2020
dc777e5
Add whatsnew entry
roberthdevries Mar 8, 2020
6612801
Remove setting the exception, does not seem to be needed
roberthdevries Mar 9, 2020
62b0cd5
Add check for expected output
roberthdevries Mar 10, 2020
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 doc/source/whatsnew/v1.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ I/O
timestamps with ``version="2.0"`` (:issue:`31652`).
- Bug in :meth:`read_csv` was raising `TypeError` when `sep=None` was used in combination with `comment` keyword (:issue:`31396`)
- Bug in :class:`HDFStore` that caused it to set to ``int64`` the dtype of a ``datetime64`` column when reading a DataFrame in Python 3 from fixed format written in Python 2 (:issue:`31750`)
- Bug in :meth:`read_excel` where a UTF-8 string with a high surrogate would cause a segmentation violation (:issue:`23809`)


Plotting
Expand Down
4 changes: 4 additions & 0 deletions pandas/_libs/src/parse_helper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ int floatify(PyObject *str, double *result, int *maybe_int) {
data = PyBytes_AS_STRING(str);
} else if (PyUnicode_Check(str)) {
tmp = PyUnicode_AsUTF8String(str);
if (tmp == NULL) {
PyErr_SetString(PyExc_TypeError, "UTF8 decode error");
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to set the error here? I think the previous call would already set and can just return, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently the 'str' object passed is a Unicode type or a subtype. Ref: https://docs.python.org/3/c-api/unicode.html
And it seems to pass that test fine. However, it fails to convert to UTF8, hence the NULL return value.
BTW, I see know that the message is wrong (decode->encode)

Copy link
Member

Choose a reason for hiding this comment

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

Let me clarify - so I am asking if you need the PyErr_SetString at all. I think the failing PyUnicode_AsUTF8String call should have already set the global error indicator / message, so I think this is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed
I tried to figure out if PyUnicode_AsUTF8String sets an exception or not, but I failed to find out.
However a manual test showed an exception so I guess we are covered here.

>>> "\udc88".encode()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'utf-8' codec can't encode character '\udc88' in position 0: surrogates not allowed

Copy link
Member

Choose a reason for hiding this comment

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

Yea that's generally the pattern for the C API functions. Quoting the guide with respect to the global error indicator:

https://docs.python.org/3/c-api/exceptions.html#exception-handling

Most C API functions don’t clear this on success, but will set it to indicate the cause of the error on failure. Most C API functions also return an error indicator, usually NULL if they are supposed to return a pointer, or -1 if they return an integer (exception: the PyArg_*() functions return 1 for success and 0 for failure).

I'm not sure which ones don't fall into the "Most" category, but good to know this one does

return -1;
}
data = PyBytes_AS_STRING(tmp);
} else {
PyErr_SetString(PyExc_TypeError, "Invalid object type");
Expand Down
Binary file added pandas/tests/io/data/excel/high_surrogate.xlsx
View file
Open in desktop
Binary file not shown.
6 changes: 6 additions & 0 deletions pandas/tests/io/excel/test_readers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,3 +1044,9 @@ def test_excel_read_binary(self, engine, read_ext):

actual = pd.read_excel(data, engine=engine)
tm.assert_frame_equal(expected, actual)

def test_excel_high_surrogate(self, read_ext):
# GH 23809

# should not produce a segmentation violation
pd.read_excel("high_surrogate.xlsx")
Copy link
Member

Choose a reason for hiding this comment

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

Can you create the expected dataframe and use tm.assert_frame_equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added