Skip to content
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

SNOW-1637945: Add support for TimedeltaIndex attributes #2193

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

sfc-gh-nkumar
Copy link
Contributor

Fixes SNOW-1637945

Add support for TimedeltaIndex attributes days, seconds, microseconds, and nanoseconds.

Copy link
Contributor

@sfc-gh-helmeleegy sfc-gh-helmeleegy left a comment

Choose a reason for hiding this comment

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

LGTM.

@sfc-gh-nkumar sfc-gh-nkumar force-pushed the nkumar-SNOW-1637945-attrs branch 2 times, most recently from d13f060 to 3f9806c Compare August 29, 2024 23:40
Copy link
Contributor

@sfc-gh-helmeleegy sfc-gh-helmeleegy left a comment

Choose a reason for hiding this comment

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

Looks good overall. I just think we need to test against null data too.

Copy link
Collaborator

@sfc-gh-azhan sfc-gh-azhan left a comment

Choose a reason for hiding this comment

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

Sorry to block this now.
@sfc-gh-mvashishtha and I had a chat today about this. Ideally we should reuse the dt.properties code to do this, we should be able to support this too:
pd.Series([1,2,3]).astype("timedelta64[ns]").dt.seconds
cc @sfc-gh-helmeleegy

@sfc-gh-nkumar
Copy link
Contributor Author

sfc-gh-nkumar commented Aug 30, 2024

Sorry to block this now. @sfc-gh-mvashishtha and I had a chat today about this. Ideally we should reuse the dt.properties code to do this, we should be able to support this too: pd.Series([1,2,3]).astype("timedelta64[ns]").dt.seconds cc @sfc-gh-helmeleegy

yeah, that is the plan. We will also use this same method to implement Series.dt.* attributes relevant for timedelta column.
There are two sets of properties. One applicable to datetime columns and one applicable to timedelta columns. dt_property implements all properties applicable to datetime column and timedelta_property implement all properteis applicable to timedelta column.
In both the scenarios code is reused between Series.dt* and [Datetime/Timedelta]Index.* attributes.

@sfc-gh-helmeleegy
Copy link
Contributor

Sorry to block this now. @sfc-gh-mvashishtha and I had a chat today about this. Ideally we should reuse the dt.properties code to do this, we should be able to support this too: pd.Series([1,2,3]).astype("timedelta64[ns]").dt.seconds cc @sfc-gh-helmeleegy

yeah, that is the plan. We will also use this same method to implement Series.dt.* attributes relevant for timedelta column. There are two sets of properties. One applicable to datetime columns and one applicable to timedelta columns. dt_property implements all properties applicable to datetime column and timedelta_property implement all properteis applicable to timedelta column. In both the scenarios code is reused between Series.dt* and [Datetime/Timedelta]Index.* attributes.

That's right. This is the plan.

@sfc-gh-nkumar sfc-gh-nkumar enabled auto-merge (squash) August 30, 2024 19:51
@sfc-gh-nkumar sfc-gh-nkumar merged commit 19538ef into main Aug 30, 2024
35 checks passed
@sfc-gh-nkumar sfc-gh-nkumar deleted the nkumar-SNOW-1637945-attrs branch August 30, 2024 20:44
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants