Repository navigation
ARROW-2432: [Python] Fix Pandas decimal type conversion with None values - #1878
BryanCutler wants to merge 7 commits into
Conversation
|
|
||
| Status DecimalMetadata::Update(PyObject* object) { | ||
| DCHECK(PyDecimal_Check(object)) << "Object is not a Python Decimal"; | ||
| bool is_decimal = PyDecimal_Check(object); |
There was a problem hiding this comment.
I don't think it's ok to do this in optimized build. DecimalMetadata expects you to pass a decimal object. @cpcloud may confirm.
There was a problem hiding this comment.
This isn't necessary because I added a check before calling Update but it does prevent a seg fault if for some reason it's called with non Decimal objects - which is not nice to get. If it's hurts an optimization though, I can remove it.
There was a problem hiding this comment.
Right now we are doing the check twice in optimized builds, which is not nice IMHO. DecimalMetadata::Update is a private API so it's up to the caller to provide appropriate input.
There was a problem hiding this comment.
So you mean remove PyDecimal_Check all together? This is only called when the type is not specified by the user and then yes, it will end doing 2 passes over the objects and checks both times if they are decimal. It might be possible to do less checks on the second pass if we keep a list of which ones are decimal objects, but I'm not sure that would be worth it.
There was a problem hiding this comment.
Fair enough, we can optimize later if we find it too slow. The conversion itself is very slow anyway :-)
| if (type_ == NULLPTR) { | ||
| for (PyObject* object : objects) { | ||
| RETURN_NOT_OK(max_decimal_metadata.Update(object)); | ||
| if (!internal::PandasObjectIsNull(object)) { |
There was a problem hiding this comment.
Do we care about accepting other NULL-like objects such as float('nan')? Otherwise object != Py_None is a much faster check.
There was a problem hiding this comment.
I'm not sure, is it possible to get NaNs from operations on Decimals? Or is that something the user might mix in somehow?
There was a problem hiding this comment.
Python decimal objects can be nan, unfortunately:
>>> import decimal
>>> decimal.Decimal('nan')
Decimal('NaN')There was a problem hiding this comment.
Seems like it could be NaN also:
In [5]: s1 = pd.Series([Decimal('1.0'), Decimal('2.0')])
In [6]: s2 = pd.Series([Decimal('2.0'), None])
In [7]: s1 / s2
Out[7]:
0 0.5
1 NaN
dtype: object
| } | ||
| } | ||
|
|
||
| type_ = |
There was a problem hiding this comment.
By the way, what happens here if all items are None? Do we have a test for that?
| Decimal128 value; | ||
| RETURN_NOT_OK(internal::DecimalFromPythonDecimal(object, decimal_type, &value)); | ||
| RETURN_NOT_OK(builder.Append(value)); | ||
| } else if (is_decimal == 0 && internal::PandasObjectIsNull(object)) { |
There was a problem hiding this comment.
Same question as above: do we care about other NULL-like values than simply None?
| series = pd.Series(data) | ||
| _check_series_roundtrip(series, type_=pa.decimal128(12, 5)) | ||
|
|
||
| def test_decimal_with_None_infer_type(self): |
There was a problem hiding this comment.
Can you also check that the expected type is inferred?
| assert arr.type == pa.binary() | ||
| _check_series_roundtrip(s, type_=pa.binary()) | ||
| # Infer type from bytearrays | ||
| _check_series_roundtrip(s) |
There was a problem hiding this comment.
But you should pass expected_pa_type here.
There was a problem hiding this comment.
ooops, right - thanks for catching that!
|
Thanks @pitrou! |
This fixes conversion of Pandas decimal types to Arrow with None values. Previously, if the type was specified, an error would occur when checking if the object was Decimal. If the type was not specified, a segmentation fault would occur when attempting to find the max precision and scale.
Added new tests which include None values for both the above cases.