-
Notifications
You must be signed in to change notification settings - Fork 119
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
fix: handle None when converting numerics to parquet #768
base: main
Are you sure you want to change the base?
Conversation
pandas_gbq/load.py
Outdated
# decimal.Decimal does not support `None` input, add support here. | ||
# https://github.com/googleapis/python-bigquery-pandas/issues/719 | ||
def convert(x): | ||
if x is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUESTION:
The original issue mentions that None
and pd.NA
are not handled appropriately.
This code seems to only address None
.
Is the intent to check for both?
IF yes... this may be a possible check that is more general AND as other edge cases arise could enable edits to the code quickly and easily.
if x is None: | |
if x in {None, pd.NA}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, thank you for catching this! I'll update the code.
# pandas.testing.assert_frame_equal() doesn't distinguish Decimal('nan') | ||
# vs. None, verify Decimal("nan") directly. | ||
# https://github.com/pandas-dev/pandas/issues/18463 | ||
assert result["A"][1].is_nan() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUESTION/PREFERENCE:
If the above question about pd.NA is answered yes, then would like to see the unit test expanded to also include feeding in a pd.NA value to ensure that it gets converted to a Decimal("NaN") value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added the unit test for this case :)
decimal.Decimal
reports an error when the input isNone
. This PR adds a lambda func to handle this case, as well as a unit test.Fixes #719 🦕