BUG: #31464 Fix error when parsing JSON list of bool into Series #33373
Conversation
…eries Add a missing exception type to the except clause, to cover the TypeError that is thrown by Cythonized array_to_datetime function when trying to convert bool to nonseconds.
| @@ -982,7 +982,7 @@ def _try_convert_to_date(self, data): | |||
| for date_unit in date_units: | |||
| try: | |||
| new_data = to_datetime(new_data, errors="raise", unit=date_unit) | |||
| except (ValueError, OverflowError): | |||
| except (ValueError, OverflowError, TypeError): | |||
There was a problem hiding this comment.
@jbrockmendel any thoughts here?
There was a problem hiding this comment.
the usual: if we can handle this case before the try/except that would be preferable
There was a problem hiding this comment.
@jbrockmendel I’ll give that approach a try.
There was a problem hiding this comment.
@jbrockmendel Pushed an alternate approach that escapes from the _try_convert_to_date() method if the dtype is bool, avoiding the try/except but adding a conditional to the method (which I don't love, but at least the method already has dtype-based conditionals).
There was a problem hiding this comment.
yes this is ok. an alternative approach is to infer_dtype on this and then dispatch. If someone wants to try that (later PR) would be ok.
pandas/io/json/_json.py
Outdated
| @@ -978,6 +978,10 @@ def _try_convert_to_date(self, data): | |||
| if not in_range.all(): | |||
| return data, False | |||
|
|
|||
| # ignore bool | |||
| if new_data.dtype == "bool": | |||
| return data, False | |||
There was a problem hiding this comment.
can you add a comment inside the if # GH#33373 ignore bool
There was a problem hiding this comment.
Done. Test failures appear to be unrelated; the FDIC site that the html tests use is returning intermittent 404s.
There was a problem hiding this comment.
Hmm actually not sure about doing it this way; we've done this type of checking in groupby and its not a great precedent. We do already catch the three errors a few lines above this - shouldn't we just do the same here?
There was a problem hiding this comment.
The first approach seems a bit simpler to me—this seems to be a case where whoever put the TypeError into the other except clauses accidentally omitted it from the one that I originally added it to. Let me know what you and @jbrockmendel decide and I’ll be happy to update accordingly.
There was a problem hiding this comment.
im happy to defer to @WillAyd in this part of the code. catching TypeError is definitely more idiomatic python, but we've definitely seen how catching too much has let bugs remain un-surfaced for a long time elsewhere
There was a problem hiding this comment.
Yea I think should catch the TypeError as done originally; would match what we have on L968 anyway
There was a problem hiding this comment.
Sounds good -- pushed the revert back to the original change. Thanks for the quick turnaround -- let me know if anything else needed.
| @@ -179,3 +179,9 @@ def test_readjson_unicode(monkeypatch): | |||
| result = read_json(path) | |||
| expected = pd.DataFrame({"£©µÀÆÖÞßéöÿ": ["АБВГДабвгд가"]}) | |||
| tm.assert_frame_equal(result, expected) | |||
|
|
|||
|
|
|||
| def test_readjson_bool_series(): | |||
There was a problem hiding this comment.
Can you move this to test_pandas and add GH 31464 as a comment in the function body?
|
thanks @jessefarnham very nice. |
Add a missing exception type to the except clause, to cover
the TypeError that is thrown by Cythonized array_to_datetime
function when trying to convert bool to nonseconds.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff