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.

Sign up for GitHub

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

Jump to bottom

[SNOW-902943]: Add support for pd.NamedAgg in groupby.agg #1432

Merged
merged 16 commits into from
May 15, 2024
Merged

[SNOW-902943]: Add support for pd.NamedAgg in groupby.agg #1432

merged 16 commits into from
May 15, 2024

Conversation

sfc-gh-rdurrani
Copy link
Contributor

@sfc-gh-rdurrani sfc-gh-rdurrani commented Apr 25, 2024 •
edited
Loading

Please answer these questions before submitting your pull requests. Thanks!

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

    Fixes #902943

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency

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

Adds support for pd.NamedAgg in groupby.agg.

@sfc-gh-rdurrani sfc-gh-rdurrani requested a review from a team as a code owner April 25, 2024 21:31
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 25, 2024 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@sfc-gh-rdurrani
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

sfc-gh-jkew
sfc-gh-jkew approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-jkew sfc-gh-jkew left a comment

Choose a reason for hiding this comment

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

ok. comments addressed in other PR on snowpandas.

@sfc-gh-mvashishtha
Copy link
Contributor

For the record, this PR follows up from https://github.com/snowflakedb/snowpandas/pull/982

sfc-gh-mvashishtha
sfc-gh-mvashishtha reviewed Apr 26, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-mvashishtha sfc-gh-mvashishtha left a comment

Choose a reason for hiding this comment

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

@sfc-gh-rdurrani some nits

src/snowflake/snowpark/modin/pandas/groupby.py Outdated Show resolved Hide resolved
tests/integ/modin/groupby/test_groupby_basic_agg.py Show resolved Hide resolved
tests/integ/modin/groupby/test_groupby_named_agg.py Outdated Show resolved Hide resolved
sfc-gh-vbudati
sfc-gh-vbudati approved these changes Apr 26, 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.

Thanks Rehan!

sfc-gh-mvashishtha
sfc-gh-mvashishtha approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-mvashishtha sfc-gh-mvashishtha 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-rdurrani sfc-gh-rdurrani enabled auto-merge (squash) April 26, 2024 22:12
sfc-gh-lspiegelberg
sfc-gh-lspiegelberg requested changes Apr 27, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-lspiegelberg sfc-gh-lspiegelberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pls surround with try: ... finally: ... to avoid affecting other tests.

tests/integ/modin/groupby/test_groupby_named_agg.py Outdated Show resolved Hide resolved
@sfc-gh-rdurrani sfc-gh-rdurrani added NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md NO-PANDAS-CHANGELOG-UPDATES NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs and removed NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md NO-PANDAS-CHANGELOG-UPDATES labels May 1, 2024
@sfc-gh-rdurrani sfc-gh-rdurrani added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label May 1, 2024
@sfc-gh-rdurrani sfc-gh-rdurrani disabled auto-merge May 1, 2024 21:57
@sfc-gh-rdurrani sfc-gh-rdurrani enabled auto-merge (squash) May 1, 2024 23:49
@sfc-gh-rdurrani sfc-gh-rdurrani merged commit c440c26 into main May 15, 2024
33 checks passed
@sfc-gh-rdurrani sfc-gh-rdurrani deleted the rdurrani/named_agg branch May 15, 2024 22:49
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sfc-gh-jkew sfc-gh-jkew sfc-gh-jkew approved these changes

@sfc-gh-mvashishtha sfc-gh-mvashishtha sfc-gh-mvashishtha approved these changes

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

@sfc-gh-lspiegelberg sfc-gh-lspiegelberg sfc-gh-lspiegelberg approved these changes

@sfc-gh-evandenberg sfc-gh-evandenberg Awaiting requested review from sfc-gh-evandenberg sfc-gh-evandenberg was automatically assigned from snowflakedb/snowpandas

Assignees
No one assigned
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sfc-gh-rdurrani @sfc-gh-mvashishtha @sfc-gh-lspiegelberg @sfc-gh-vbudati @sfc-gh-jkew