[SNOW-902943]: Add support for pd.NamedAgg in DataFrame and Series.agg #1652

sfc-gh-rdurrani · 2024-05-22T01:22:31Z

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

Fixes SNOW-902943
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.
Please describe how your code solves the related issue.

Adds support for NamedAggregations for df and series.agg

src/snowflake/snowpark/modin/pandas/groupby.py

CHANGELOG.md

tests/integ/modin/frame/test_aggregate.py

src/snowflake/snowpark/modin/pandas/base.py

sfc-gh-yzou · 2024-05-23T16:48:44Z

src/snowflake/snowpark/modin/pandas/base.py

+                self, allow_duplication=False, axis=axis, is_from_agg=True, **kwargs
+            )
+        else:
+            func = validate_and_try_convert_agg_func_arg_func_to_str(


@sfc-gh-joshi with the modin upstreaming work, I think we probably should start moving many of those conversion to backend, just leave basic checking at frontend

Yes, though our frontend implementation of aggregate already diverges pretty heavily from the modin version. We might want to use the extension API to overwrite aggregate when we start the migration.

src/snowflake/snowpark/modin/pandas/groupby.py

src/snowflake/snowpark/modin/pandas/utils.py

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py

sfc-gh-joshi

I reviewed most of the frontend/argument parsing logic, and haven't looked at the query compiler changes yet since I left a few questions about column ordering that I'd like to understand first.

sfc-gh-joshi · 2024-05-23T17:29:02Z

src/snowflake/snowpark/modin/pandas/base.py

+                self, allow_duplication=False, axis=axis, is_from_agg=True, **kwargs
+            )
+        else:
+            func = validate_and_try_convert_agg_func_arg_func_to_str(


Yes, though our frontend implementation of aggregate already diverges pretty heavily from the modin version. We might want to use the extension API to overwrite aggregate when we start the migration.

src/snowflake/snowpark/modin/pandas/base.py

src/snowflake/snowpark/modin/pandas/groupby.py

src/snowflake/snowpark/modin/plugin/_internal/aggregation_utils.py

tests/integ/modin/frame/test_aggregate.py

sfc-gh-joshi

Thanks! I left a few more minor comments/suggestions.

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py

tests/integ/modin/frame/test_aggregate.py

tests/integ/modin/groupby/test_groupby_named_agg.py

tests/integ/modin/groupby/test_groupby_series.py

…ompiler.py Co-authored-by: Jonathan Shi <149419494+sfc-gh-joshi@users.noreply.github.com>

CHANGELOG.md

src/snowflake/snowpark/modin/pandas/base.py

src/snowflake/snowpark/modin/plugin/_internal/aggregation_utils.py

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py

sfc-gh-yzou · 2024-05-24T00:57:25Z

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py

+        # and if not, reorder them.
+        if uses_named_aggs:
+            correct_ordering = list(agg_kwargs.keys())
+            if correct_ordering != new_data_column_pandas_labels:


That seems fine for now since most of the code is done in this way now. Ideally, i think we should only do function string conversion at frontend, and then pass them directly to query compiler (the str conversion probably should also go to query backend directly also) when we are getting the column_to_agg_func, it should be in correct order based on the input. pass the order as part of agg_kwargs seems hacky to me.
@sfc-gh-joshi when we do the modin upstreaming support for this, let's talk about this.

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py

src/snowflake/snowpark/modin/pandas/base.py

tests/integ/modin/frame/test_aggregate.py

CHANGELOG.md

[SNOW-902943]: Add support for pd.NamedAgg in DataFrame and Series.agg

17019e9

sfc-gh-rdurrani requested a review from a team as a code owner May 22, 2024 01:22

sfc-gh-rdurrani requested review from sfc-gh-yzou and sfc-gh-dpetersohn May 22, 2024 01:22

sfc-gh-rdurrani marked this pull request as draft May 22, 2024 01:22

sfc-gh-rdurrani added 3 commits May 22, 2024 14:20

Add testing

32ed3a6

Merge branch 'main' into rdurrani-agg-named

3493b21

Add changelog

60dc117

sfc-gh-rdurrani added the NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs label May 22, 2024

sfc-gh-rdurrani commented May 22, 2024

View reviewed changes

src/snowflake/snowpark/modin/pandas/groupby.py Show resolved Hide resolved

Finalize tests

4156af2

sfc-gh-rdurrani marked this pull request as ready for review May 23, 2024 00:13

Merge branch 'main' into rdurrani-agg-named

b8fb47e

sfc-gh-rdurrani enabled auto-merge (squash) May 23, 2024 00:19

sfc-gh-rdurrani disabled auto-merge May 23, 2024 00:19

Fix bug

679e619

sfc-gh-yzou reviewed May 23, 2024

View reviewed changes

sfc-gh-joshi reviewed May 23, 2024

View reviewed changes

sfc-gh-rdurrani added 5 commits May 23, 2024 14:26

Address review comments

aabc228

Merge branch 'main' into rdurrani-agg-named

86fe6cc

Add extra test

dfc660a

Add pd.NamedAgg tests

7282cda

Add some more tests to verify error parity

9bffb90

sfc-gh-joshi reviewed May 23, 2024

View reviewed changes

Update src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_c…

c1dd9e3

…ompiler.py Co-authored-by: Jonathan Shi <149419494+sfc-gh-joshi@users.noreply.github.com>

sfc-gh-joshi approved these changes May 23, 2024

View reviewed changes

sfc-gh-rdurrani added 2 commits May 23, 2024 16:46

Address review comments

4c514b0

Merge branch 'main' into rdurrani-agg-named

1de7714

github-actions bot added the snowpark-pandas label May 23, 2024

sfc-gh-yzou reviewed May 24, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

sfc-gh-rdurrani added 2 commits May 24, 2024 19:04

Address review comments

abfd6ca

Merge branch 'main' into rdurrani-agg-named (rebase onto 1.18)

9c26c31

sfc-gh-yzou reviewed May 28, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

sfc-gh-rdurrani added 3 commits May 29, 2024 14:11

Address review comments

d298cdc

Merge branch 'main' into rdurrani-agg-named

60eccc6

Fix test

1d63182

sfc-gh-yzou approved these changes May 29, 2024

View reviewed changes

sfc-gh-rdurrani merged commit 6a1bd53 into main May 29, 2024
37 checks passed

sfc-gh-rdurrani deleted the rdurrani-agg-named branch May 29, 2024 23:08

github-actions bot locked and limited conversation to collaborators May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SNOW-902943]: Add support for pd.NamedAgg in DataFrame and Series.agg #1652

[SNOW-902943]: Add support for pd.NamedAgg in DataFrame and Series.agg #1652

sfc-gh-rdurrani commented May 22, 2024 •

edited

Loading

sfc-gh-yzou May 23, 2024

sfc-gh-joshi May 23, 2024

sfc-gh-joshi left a comment

sfc-gh-joshi May 23, 2024

sfc-gh-joshi left a comment

sfc-gh-yzou May 24, 2024

[SNOW-902943]: Add support for pd.NamedAgg in DataFrame and Series.agg #1652

[SNOW-902943]: Add support for pd.NamedAgg in DataFrame and Series.agg #1652

Conversation

sfc-gh-rdurrani commented May 22, 2024 • edited Loading

sfc-gh-yzou May 23, 2024

Choose a reason for hiding this comment

sfc-gh-joshi May 23, 2024

Choose a reason for hiding this comment

sfc-gh-joshi left a comment

Choose a reason for hiding this comment

sfc-gh-joshi May 23, 2024

Choose a reason for hiding this comment

sfc-gh-joshi left a comment

Choose a reason for hiding this comment

sfc-gh-yzou May 24, 2024

Choose a reason for hiding this comment

sfc-gh-rdurrani commented May 22, 2024 •

edited

Loading