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-1618349: Add support for pd.merge_asof #2095

Merged
merged 20 commits into from
Aug 22, 2024
Merged

SNOW-1618349: Add support for pd.merge_asof #2095

merged 20 commits into from
Aug 22, 2024

Conversation

sfc-gh-nkrishna
Copy link
Contributor

@sfc-gh-nkrishna sfc-gh-nkrishna commented Aug 15, 2024 •
edited
Loading

Fixes SNOW-1618349

This PR adds initial support for pd.merge_asof -- parameters by, left_by, right_by, left_index, right_index, suffixes, and tolerance are not yet supported. Additionally direction=nearest is not yet supported but this can be done in a follow-up PR.

@sfc-gh-nkrishna sfc-gh-nkrishna requested a review from a team as a code owner August 15, 2024 00:07
@sfc-gh-nkrishna sfc-gh-nkrishna changed the title SNOW-1518349: Add support for pd.merge_asof SNOW-1618349: Add support for pd.merge_asof Aug 15, 2024
sfc-gh-helmeleegy
sfc-gh-helmeleegy reviewed Aug 15, 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.

Looks good overall. Just a few nits. Thanks for the quick turnaround.

sfc-gh-rdurrani
sfc-gh-rdurrani requested changes Aug 15, 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.

left some comments + requests

src/snowflake/snowpark/modin/pandas/general.py Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated Show resolved Hide resolved
tests/integ/modin/test_merge_asof.py Outdated Show resolved Hide resolved
tests/integ/modin/test_merge_asof.py Outdated Show resolved Hide resolved
tests/integ/modin/test_merge_asof.py Show resolved Hide resolved
tests/integ/modin/test_merge_asof.py Show resolved Hide resolved
sfc-gh-rdurrani
sfc-gh-rdurrani reviewed Aug 15, 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.

LGTM - left a couple quick q's tho!

src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated Show resolved Hide resolved
@sfc-gh-nkrishna
SNOW-1518349: Add support for merge_asof
d3cfae0 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
fix test failures
ab3b0a5 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
respond to comments
2744d0a 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
use valid data indices instead of invalid ones
2bfb869 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
fix comments
a2a4d4c 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
comment fix 2
969bb3c 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
make more concise
db0aea3 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
fix doctest failures
c145b9e 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
fix build doc
9375a4b 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
address comments
2fa62bf 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
address comments 2
6984760 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-yzou sfc-gh-yzou self-requested a review August 16, 2024 20:44
@sfc-gh-nkrishna sfc-gh-nkrishna marked this pull request as draft August 16, 2024 21:44
@sfc-gh-nkrishna
refactor to use join_utils
15ddda1 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna sfc-gh-nkrishna marked this pull request as ready for review August 20, 2024 19:23
@sfc-gh-nkrishna
Merge branch 'main' into nkrishna-SNOW-1518349-merge-asof
8f12259
@sfc-gh-nkrishna
fix changelog
b08d63e 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
fix docstring
ed8c434 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
sfc-gh-rdurrani
sfc-gh-rdurrani requested changes Aug 20, 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.

I think that we should add support for by, since it won't be a whole lot of extra code.

src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Show resolved Hide resolved
sfc-gh-rdurrani
sfc-gh-rdurrani approved these changes Aug 20, 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.

LGTM!

sfc-gh-yzou
sfc-gh-yzou reviewed Aug 21, 2024
View reviewed changes
src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py Outdated Show resolved Hide resolved
tests/integ/modin/test_merge_asof.py Show resolved Hide resolved
tests/integ/modin/test_merge_asof.py Show resolved Hide resolved
@sfc-gh-nkrishna
address Yun comments
a5e46fd 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
common util function coalesce
afa9b30 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
remove type ignore
0a32d11 
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-nkrishna
Merge branch 'main' into nkrishna-SNOW-1518349-merge-asof
2121c68
@sfc-gh-nkrishna
Copy link
Contributor Author

@sfc-gh-yzou Could you take another pass? Addressed all comments

@sfc-gh-nkrishna sfc-gh-nkrishna merged commit 14d9c61 into main Aug 22, 2024
35 checks passed
@sfc-gh-nkrishna sfc-gh-nkrishna deleted the nkrishna-SNOW-1518349-merge-asof branch August 22, 2024 18:14
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sfc-gh-yzou sfc-gh-yzou sfc-gh-yzou approved these changes

@sfc-gh-rdurrani sfc-gh-rdurrani sfc-gh-rdurrani approved these changes

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

@sfc-gh-nkumar sfc-gh-nkumar Awaiting requested review from sfc-gh-nkumar sfc-gh-nkumar is a code owner automatically assigned from snowflakedb/snowpark-pandas-api

@sfc-gh-evandenberg sfc-gh-evandenberg Awaiting requested review from sfc-gh-evandenberg sfc-gh-evandenberg is a code owner automatically assigned from snowflakedb/snowpark-pandas-api

Assignees

@sfc-gh-nkumar sfc-gh-nkumar

Labels
snowpark-pandas
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sfc-gh-nkrishna @sfc-gh-yzou @sfc-gh-rdurrani @sfc-gh-helmeleegy @sfc-gh-nkumar