fix: dataframe fillna with scalar. #1132
Conversation
There was a problem hiding this comment.
LGTM overall with a nit question:
If I understand correctly, before this Pull Request, using fillna with a string scalar would result in a NotImplemented error. If that's the case, perhaps the title should start with feat: instead of fix:?
| how: str = "outer", | ||
| reverse: bool = False, | ||
| ): | ||
| if isinstance(other, (float, int, bool)): |
There was a problem hiding this comment.
I feel like just adding another type here is a narrow fix - what we really want to do is determine - is this type interpretable as a supported scalar. This could include datatime objects, decimal, etc as well. We should probably have a single definition of this somewhere
There was a problem hiding this comment.
Added a LOCAL_SCALAR_TYPES constant, all types are from infer_literal_type function, should be supported.
bigframes/dataframe.py
Outdated
|
|
||
| def _apply_scalar_binop( | ||
| self, other: float | int, op: ops.BinaryOp, reverse: bool = False | ||
| self, other: float | int | bool | str, op: ops.BinaryOp, reverse: bool = False |
There was a problem hiding this comment.
Should we use bigframes.dtypes.LOCAL_SCALAR_TYPES here too?
There was a problem hiding this comment.
Updated, added a new constant for annotation.
| datetime.date, | ||
| datetime.time, | ||
| ] | ||
| LOCAL_SCALAR_TYPES = typing.get_args(LOCAL_SCALAR_TYPE) |
There was a problem hiding this comment.
I have a feeling we will just give up at a certain point as we add more types
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕