Skip to content

SNOW-1757439, SNOW-1757441: Support DataFrameGroupBy/SeriesGroupBy.pct_change#2625

Merged
sfc-gh-joshi merged 9 commits intomainfrom
joshi-SNOW-1757439-groupby-pct_change
Nov 22, 2024
Merged

SNOW-1757439, SNOW-1757441: Support DataFrameGroupBy/SeriesGroupBy.pct_change#2625
sfc-gh-joshi merged 9 commits intomainfrom
joshi-SNOW-1757439-groupby-pct_change

Conversation

@sfc-gh-joshi
Copy link
Contributor

@sfc-gh-joshi sfc-gh-joshi commented Nov 14, 2024 •
edited
Loading

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1757439, SNOW-1757441

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines

  3. Please describe how your code solves the related issue.

This PR implements GroupBy.pct_change on top of the existing pct_change API.

Leaving changelog un-updated until 1.25 is cut/merged back.

@sfc-gh-joshi sfc-gh-joshi force-pushed the joshi-SNOW-1757439-groupby-pct_change branch from 9cc7cdc to bef44c8 Compare November 14, 2024 19:47
@sfc-gh-joshi sfc-gh-joshi marked this pull request as ready for review November 14, 2024 20:49
@sfc-gh-joshi sfc-gh-joshi requested a review from a team as a code owner November 14, 2024 20:49
@sfc-gh-joshi sfc-gh-joshi force-pushed the joshi-SNOW-1757439-groupby-pct_change branch from 8af07b0 to 04b63bd Compare November 14, 2024 22:46
@sfc-gh-joshi sfc-gh-joshi force-pushed the joshi-SNOW-1757439-groupby-pct_change branch from 04b63bd to bcc08e1 Compare November 15, 2024 19:03
sfc-gh-vbudati
sfc-gh-vbudati reviewed Nov 20, 2024
View reviewed changes
src/snowflake/snowpark/modin/plugin/extensions/groupby_overrides.py
):
# TODO: SNOW-1063349: Modin upgrade - modin.pandas.groupby.DataFrameGroupBy functions
ErrorMessage.method_not_implemented_error(name="pct_change", class_="GroupBy")
if fill_method not in (no_default, None) or limit is not no_default:
Copy link
Contributor

@sfc-gh-vbudati sfc-gh-vbudati Nov 20, 2024

Choose a reason for hiding this comment

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

why not separate this into two different warnings? It would make the error message easier to read and less verbose.

Copy link
Contributor Author

@sfc-gh-joshi sfc-gh-joshi Nov 21, 2024

Choose a reason for hiding this comment

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

I copied this from modin, which copied the message from pandas.

tests/integ/modin/groupby/test_pct_change.py
"params",
[
{"limit": 1},
{"freq": 1},
Copy link
Contributor

@sfc-gh-vbudati sfc-gh-vbudati Nov 20, 2024

Choose a reason for hiding this comment

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

add test for axis=1?

Copy link
Contributor

@sfc-gh-helmeleegy sfc-gh-helmeleegy Nov 21, 2024

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

@sfc-gh-joshi sfc-gh-joshi Nov 21, 2024

Choose a reason for hiding this comment

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

Added!

sfc-gh-helmeleegy
sfc-gh-helmeleegy reviewed Nov 21, 2024
View reviewed changes
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.

PR looks good to me overall. Just please add the needed extra test coverage.

sfc-gh-rdurrani
sfc-gh-rdurrani reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-rdurrani sfc-gh-rdurrani left a comment

Choose a reason for hiding this comment

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

overall lgtm, left some comments.

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated
Copy link
Contributor

@sfc-gh-rdurrani sfc-gh-rdurrani Nov 21, 2024

Choose a reason for hiding this comment

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

shouldn't we error if the length isn't greater than 0? That would mean that we got to this point in the code and have a label that doesn't have any corresponding columns in the dataframe?

Copy link
Contributor Author

@sfc-gh-joshi sfc-gh-joshi Nov 21, 2024

Choose a reason for hiding this comment

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

Good point. This case should be caught gracefully by the groupby frontend if the user specifies a missing label, so I went ahead and just removed this line.

tests/integ/modin/groupby/test_pct_change.py
]


@pytest.mark.parametrize("periods", [0, 1, 2, -1])
Copy link
Contributor

@sfc-gh-rdurrani sfc-gh-rdurrani Nov 21, 2024

Choose a reason for hiding this comment

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

We should get some tests with NA values to make sure they are propagated (or filled correctly if fill_method is specified).

Copy link
Contributor Author

@sfc-gh-joshi sfc-gh-joshi Nov 21, 2024

Choose a reason for hiding this comment

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

Added! (also caught some bugs with it)

sfc-gh-helmeleegy
sfc-gh-helmeleegy approved these changes Nov 22, 2024
View reviewed changes
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, thanks!

sfc-gh-vbudati
sfc-gh-vbudati approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-vbudati sfc-gh-vbudati left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Jonathan!!

@sfc-gh-joshi sfc-gh-joshi enabled auto-merge (squash) November 22, 2024 19:12
@sfc-gh-joshi sfc-gh-joshi merged commit de91397 into main Nov 22, 2024
@sfc-gh-joshi sfc-gh-joshi deleted the joshi-SNOW-1757439-groupby-pct_change branch November 22, 2024 19:34
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sfc-gh-vbudati sfc-gh-vbudati sfc-gh-vbudati approved these changes

@sfc-gh-helmeleegy sfc-gh-helmeleegy sfc-gh-helmeleegy approved these changes

+1 more reviewer

@sfc-gh-rdurrani sfc-gh-rdurrani sfc-gh-rdurrani left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

snowpark-pandas

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sfc-gh-joshi @sfc-gh-vbudati @sfc-gh-rdurrani @sfc-gh-helmeleegy

Comments