Skip to content

Conversation

Genesis929
Copy link
Collaborator

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Nov 5, 2024
@Genesis929 Genesis929 marked this pull request as ready for review November 5, 2024 20:49
@Genesis929 Genesis929 requested review from a team as code owners November 5, 2024 20:49
@Genesis929 Genesis929 requested a review from shobsi November 5, 2024 20:57
Copy link
Contributor

@chelsea-lin chelsea-lin left a comment

Choose a reason for hiding this comment

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

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)):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a LOCAL_SCALAR_TYPES constant, all types are from infer_literal_type function, should be supported.

@Genesis929 Genesis929 changed the title fix: dataframe fillna with string scalar. fix: dataframe fillna with scalar. Nov 6, 2024

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
Copy link
Contributor

@shobsi shobsi Nov 7, 2024

Choose a reason for hiding this comment

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

Should we use bigframes.dtypes.LOCAL_SCALAR_TYPES here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, added a new constant for annotation.

@Genesis929 Genesis929 requested a review from shobsi November 12, 2024 19:43
datetime.date,
datetime.time,
]
LOCAL_SCALAR_TYPES = typing.get_args(LOCAL_SCALAR_TYPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling we will just give up at a certain point as we add more types

@Genesis929 Genesis929 merged commit 37f8c32 into main Nov 13, 2024
22 of 23 checks passed
@Genesis929 Genesis929 deleted the fillna_fix_huanc branch November 13, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants