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.

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-1722641: Add support for Series.between #2775

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

sfc-gh-joshi
Copy link
Contributor

@sfc-gh-joshi sfc-gh-joshi commented Dec 17, 2024

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

    Fixes SNOW-1722641

  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.

Implements Series.between. The frontend implementation uses modin's implementation, which passes the modin.pandas.Series object to the native pandas method, which directly uses comparison operators to implement the method.

Copy link
Contributor

@sfc-gh-jjiao sfc-gh-jjiao left a comment

Choose a reason for hiding this comment

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

This is just fall back to native pandas, right?

@sfc-gh-joshi
Copy link
Contributor Author

This is just fall back to native pandas, right?

Yes, but not in a way that causes extra materialization (the native pandas method just uses comparison operators and &, so we can preserve lazy evaluation).

@sfc-gh-helmeleegy
Copy link
Contributor

This is just fall back to native pandas, right?

Yes, but not in a way that causes extra materialization (the native pandas method just uses comparison operators and &, so we can preserve lazy evaluation).

No, native pandas is not being used here. This was just falling back to modin implementation which itself routes to other supported Snowpark pandas methods (comparison operators).

@sfc-gh-joshi
Copy link
Contributor Author

No, native pandas is not being used here.

The modin frontend implementation (here) actually calls a native pandas method (here), but the native pandas method routes to Snowpark pandas comparison operators.

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.

Nice work. Thanks, Jonathan!

@sfc-gh-joshi sfc-gh-joshi force-pushed the joshi-SNOW-1722641-series-between branch from 0f4fda7 to bcff17a Compare December 19, 2024 01:36
@sfc-gh-joshi sfc-gh-joshi enabled auto-merge (squash) December 19, 2024 01:37
@sfc-gh-joshi sfc-gh-joshi force-pushed the joshi-SNOW-1722641-series-between branch from bcff17a to 4039efb Compare December 19, 2024 19:37
@sfc-gh-joshi sfc-gh-joshi merged commit be0ae7b into main Dec 19, 2024
42 of 43 checks passed
@sfc-gh-joshi sfc-gh-joshi deleted the joshi-SNOW-1722641-series-between branch December 19, 2024 20:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants