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-1625368 SNOW-1634393 Support indexing with Timedelta data columns #2141

Merged
merged 5 commits into from
Aug 22, 2024

Conversation

sfc-gh-azhan
Copy link
Collaborator

@sfc-gh-azhan sfc-gh-azhan commented Aug 21, 2024

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

    SNOW-1625368 Support indexing with Timedelta data columns

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

    Please write a short description of how your code change solves the related issue.

Support indexing with Timedelta data columns:

  • Added sufficient tests cases for indexing including get and set values with the same timedelta type or different types.
  • To achieve this, I updated several util methods related to transpose, join too.
  • Also, I need to refactor SnowparkPandasType since there are some confusing on type object and class there.

This pull request includes several changes to enhance support for Timedelta and improve type safety in the Snowflake Snowpark Modin plugin. The most important changes are summarized below:

Enhancements to Timedelta Support:

Type Safety Improvements:

  • Added assertions to check that data_column_types and index_column_types are instances of SnowparkPandasType in _create_snowflake_quoted_identifier_to_snowpark_pandas_type. (src/snowflake/snowpark/modin/plugin/_internal/frame.py: src/snowflake/snowpark/modin/plugin/_internal/frame.pyR92-R106)
  • Updated project_columns to include an optional column_types parameter and ensure its length matches pandas_labels. (src/snowflake/snowpark/modin/plugin/_internal/frame.py: [1] [2] [3]
  • Modified set_frame_2d_labels and set_frame_2d_positional to handle SnowparkPandasColumn and SnowparkPandasType correctly. (src/snowflake/snowpark/modin/plugin/_internal/indexing_utils.py: [1] [2] [3] [4] [5] [6]
  • Updated get_item_series_as_single_row_frame to include SnowparkPandasType. (src/snowflake/snowpark/modin/plugin/_internal/indexing_utils.py: src/snowflake/snowpark/modin/plugin/_internal/indexing_utils.pyR3065)
  • Enhanced _create_internal_frame_with_join_or_align_result to handle data_column_types and index_column_types during joins. (src/snowflake/snowpark/modin/plugin/_internal/join_utils.py: [1] [2] [3]

@sfc-gh-azhan sfc-gh-azhan added the NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs label Aug 21, 2024
@sfc-gh-azhan sfc-gh-azhan requested a review from a team as a code owner August 21, 2024 22:10
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.

some minor comments.

@sfc-gh-azhan sfc-gh-azhan force-pushed the azhan-timedelta-indexing-1625368 branch from f5536b0 to 684a8af Compare August 21, 2024 22:56
Copy link
Contributor

@sfc-gh-nkrishna sfc-gh-nkrishna left a comment

Choose a reason for hiding this comment

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

LGTM! Nice :)

@sfc-gh-azhan sfc-gh-azhan force-pushed the azhan-timedelta-indexing-1625368 branch from 7f045aa to 1ce1d99 Compare August 22, 2024 16:31
@sfc-gh-mvashishtha
Copy link
Contributor

@sfc-gh-azhan I restarted CI on this pr because I assumed it was a flake, but it looks legitimate: https://github.com/snowflakedb/snowpark-python/actions/runs/10512030861/job/29124497386?pr=2141

@sfc-gh-azhan
Copy link
Collaborator Author

@sfc-gh-azhan I restarted CI on this pr because I assumed it was a flake, but it looks legitimate: https://github.com/snowflakedb/snowpark-python/actions/runs/10512030861/job/29124497386?pr=2141

It was a window specific failure. Fixed.

@sfc-gh-azhan sfc-gh-azhan changed the title SNOW-1625468 Support indexing with Timedelta data columns SNOW-1625468 SNOW-1634393 Support indexing with Timedelta data columns Aug 22, 2024
@sfc-gh-azhan sfc-gh-azhan merged commit b8e2b13 into main Aug 22, 2024
35 checks passed
@sfc-gh-azhan sfc-gh-azhan deleted the azhan-timedelta-indexing-1625368 branch August 22, 2024 18:06
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
@sfc-gh-azhan sfc-gh-azhan changed the title SNOW-1625468 SNOW-1634393 Support indexing with Timedelta data columns SNOW-1625368 SNOW-1634393 Support indexing with Timedelta data columns Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs snowpark-pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants