-
Notifications
You must be signed in to change notification settings - Fork 109
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-1618349: Add support for pd.merge_asof
#2095
Conversation
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Show resolved
Hide resolved
pd.merge_asof
pd.merge_asof
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
There was a problem hiding this 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.
There was a problem hiding this 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/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
There was a problem hiding this 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
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
5f83307
to
6984760
Compare
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
Show resolved
Hide resolved
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
There was a problem hiding this 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/compiler/snowflake_query_compiler.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
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
Outdated
Show resolved
Hide resolved
src/snowflake/snowpark/modin/plugin/_internal/ordered_dataframe.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
Signed-off-by: Naren Krishna <naren.krishna@snowflake.com>
@sfc-gh-yzou Could you take another pass? Addressed all comments |
Fixes SNOW-1618349
This PR adds initial support for
pd.merge_asof
-- parametersby
,left_by
,right_by
,left_index
,right_index
,suffixes
, andtolerance
are not yet supported. Additionallydirection=nearest
is not yet supported but this can be done in a follow-up PR.