-
Notifications
You must be signed in to change notification settings - Fork 49
refactor: Simplify local arrow data management #1591
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
Conversation
b8b6438
to
8d504a5
Compare
8d504a5
to
aade6cb
Compare
bigframes/core/local_data.py
Outdated
total_bytes: int | ||
row_count: int | ||
|
||
@staticmethod |
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.
nit: It feels like @classmethod is a better fit for factory methods like this
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.
yeah, thats true, changed in new revision
# allows directly using values out of a duration arrow array | ||
if isinstance(value, datetime.timedelta): | ||
value = ( | ||
(value.days * 3600 * 24) + value.seconds |
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.
Could we use value.total_seconds()
? It might simplify the expression a bit
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.
eh, floats are evil though, also I copied from https://github.com/googleapis/python-bigquery-dataframes/blame/2ce891fcd5bfd9f093fbcbb1ea35158d2bf9d8b9/bigframes/core/utils.py#L219-L222
82b6816
to
8dcc03d
Compare
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> 🦕