Skip to content

SNOW-1794509 Merging from main into decoder branch. #2826

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

Merged
merged 26 commits into from
Jan 13, 2025

Conversation

sfc-gh-batur
Copy link
Contributor

@sfc-gh-batur sfc-gh-batur commented Jan 6, 2025

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

    Fixes SNOW-1794509

  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.

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

sfc-gh-joshi and others added 14 commits December 19, 2024 20:40
<!---
Please answer these questions before creating your pull request. Thanks!
--->

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

   <!---
   In this section, please add a Snowflake Jira issue number.

Note that if a corresponding GitHub issue exists, you should still
include
   the Snowflake Jira issue number. For example, for GitHub issue
#1400, you should
   add "SNOW-1335071" here.
    --->

   Fixes SNOW-1722641

2. Fill out the following pre-review checklist:

- [x] 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.
- [x] I acknowledge that I have ensured my changes to be thread-safe.
Follow the link for more information: [Thread-safe Developer
Guidelines](https://github.com/snowflakedb/snowpark-python/blob/main/CONTRIBUTING.md#thread-safe-development)

3. Please describe how your code solves the related issue.

Implements `Series.between`. The frontend implementation uses [modin's
implementation](https://github.com/modin-project/modin/blob/1c4d173d3b2c44a1c1b5d5516552c7717b26de32/modin/pandas/series.py#L795),
which passes the `modin.pandas.Series` object to the [native pandas
method](https://github.com/pandas-dev/pandas/blob/9fe33bcbca79e098f9ba8ffd9fcf95440b95032b/pandas/core/series.py#L5362-L5380),
which directly uses comparison operators to implement the method.
<!---
Please answer these questions before creating your pull request. Thanks!
--->

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

   <!---
   In this section, please add a Snowflake Jira issue number.

Note that if a corresponding GitHub issue exists, you should still
include
   the Snowflake Jira issue number. For example, for GitHub issue
#1400, you should
   add "SNOW-1335071" here.
    --->

   Fixes SNOW-1866086

2. Fill out the following pre-review checklist:

- [x] 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](https://github.com/snowflakedb/snowpark-python/blob/main/CONTRIBUTING.md#thread-safe-development)

3. Please describe how your code solves the related issue.

   Fix test_dt_accessor.py::test_strftime in Windows.
…e_nulls` (#2800)

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

   Fixes SNOW-1866100

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.
- [x] I acknowledge that I have ensured my changes to be thread-safe.
Follow the link for more information: [Thread-safe Developer
Guidelines](https://github.com/snowflakedb/snowpark-python/blob/main/CONTRIBUTING.md#thread-safe-development)

3. Please describe how your code solves the related issue.
Update DataFrame.unpivot AST encoding to include `include_nulls`
…perties (#2795)

<!---
Please answer these questions before creating your pull request. Thanks!
--->

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

   <!---
   In this section, please add a Snowflake Jira issue number.

Note that if a corresponding GitHub issue exists, you should still
include
   the Snowflake Jira issue number. For example, for GitHub issue
#1400, you should
   add "SNOW-1335071" here.
    --->

   Fixes SNOW-1865997

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](https://github.com/snowflakedb/snowpark-python/blob/main/CONTRIBUTING.md#thread-safe-development)

3. Please describe how your code solves the related issue.

Use docstrings folder for DatetimeIndex methods and properties.
Docstrings for TimedeltaIndex will be treated similarly in a follow-up
PR
- Move the existing scikit-learn interoperability test into the
`interoperability` test folder.
- Refactor the test file so that it groups test cases into scikit-learn
method categories, like classification and regression.
- Add test cases so we have one test case for each category.
- Remove the scikit-learn version pin for the modin dev environment.
Once we implemented the interchange protocol, we no longer needed that
pin.
- Document the support for the methods that we've tested.
- Reorganize the interoperability documentation page so that it follows
the rst guidelines for sections and subsections.

Signed-off-by: sfc-gh-mvashishtha <mahesh.vashishtha@snowflake.com>
Co-authored-by: Hazem Elmeleegy <hazem.elmeleegy@snowflake.com>
@sfc-gh-batur sfc-gh-batur self-assigned this Jan 6, 2025
@sfc-gh-batur sfc-gh-batur requested review from a team as code owners January 6, 2025 21:34
@sfc-gh-batur sfc-gh-batur requested review from sfc-gh-rdurrani, sfc-gh-aling, sfc-gh-aalam and sfc-gh-jrose and removed request for a team January 6, 2025 21:34
Copy link

github-actions bot commented Jan 6, 2025

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

sfc-gh-oplaton and others added 12 commits January 6, 2025 14:10
<!---
Please answer these questions before creating your pull request. Thanks!
--->

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

   <!---
   In this section, please add a Snowflake Jira issue number.

Note that if a corresponding GitHub issue exists, you should still
include
   the Snowflake Jira issue number. For example, for GitHub issue
#1400, you should
   add "SNOW-1335071" here.
    --->

   Fixes SNOW-1621205

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.
- [x] I acknowledge that I have ensured my changes to be thread-safe.
Follow the link for more information: [Thread-safe Developer
Guidelines](https://github.com/snowflakedb/snowpark-python/blob/main/CONTRIBUTING.md#thread-safe-development)

3. Please describe how your code solves the related issue.

This is the client-side change corresponding to
snowflakedb/snowflake#240557

Unify Snowflake object name handling in the Snowpark AST.
Remove `FnName` and `SpTableName`. They both had `Flat` and `Structured`
variants, but ultimately designate Snowflake object names.
Introduce `data SpName` and `entity SpNameRef` for referring to
Snowflake objects by relative or fully qualified name.
Update `FnNameRefExpr`.
Use `SpNameRef` in a few places that used to use `List[String]`.
pandas 2.2.3 is available in anaconda, so we can once again run the tests that try to use UD(T)Fs.

Fixes SNOW-1739034

Signed-off-by: sfc-gh-mvashishtha <mahesh.vashishtha@snowflake.com>
…ap (#2821)

SNOW-1852925 & SNOW-1852928

<!---
Please answer these questions before creating your pull request. Thanks!
--->

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

   <!---
   In this section, please add a Snowflake Jira issue number.

Note that if a corresponding GitHub issue exists, you should still
include
   the Snowflake Jira issue number. For example, for GitHub issue
#1400, you should
   add "SNOW-1335071" here.
    --->

   Fixes SNOW-NNNNNNN

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](https://github.com/snowflakedb/snowpark-python/blob/main/CONTRIBUTING.md#thread-safe-development)

3. Please describe how your code solves the related issue.

   Add type inference for DataFrame.map, Series.apply and Series.map
return table_name.sp_table_name_flat.name
elif hasattr(table_name, "sp_table_name_structured"):
return table_name.sp_table_name_structured.name
if table_name.name.HasField("sp_name_flat"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this issue pops up here but I believe that using HasField raises a ValueError if the field is not defined in the protobuf object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation says that calling it on a "non-message" type will throw an error. In this case name is a message type - so I think we should be ok. hasattr doesn't work because both the fields (flat and structured) are present and it returns true for both.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, thanks Bala!

return fn_name.fn_name_structured.name
else:
raise ValueError("Function name not found in proto.FnName")
def convert_name_to_list(self, name: any) -> List:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to not unify this method with decode_name_expr? I see that it is called only after decode_name_expr is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But not always. There are places where it is not called and I thought it might be better to have separate functions rather than a single function with a parameter...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm fair point since, we can keep it as it is then

Copy link
Contributor

@sfc-gh-vbudati sfc-gh-vbudati 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 for most part, I have a couple questions though regarding the decoder. Thanks Bala!

@sfc-gh-evandenberg
Copy link
Collaborator

Can you fix up the description? It doesn't contain the jira number.

@sfc-gh-batur
Copy link
Contributor Author

Can you fix up the description? It doesn't contain the jira number.

Merges require Jira numbers ?

@sfc-gh-batur sfc-gh-batur changed the title Merging from main into decoder branch. SNOW-1794509 Merging from main into decoder branch. Jan 13, 2025
@sfc-gh-batur sfc-gh-batur merged commit ae5fd81 into vbudati/SNOW-1794510-merge-decoder Jan 13, 2025
23 of 40 checks passed
@sfc-gh-batur sfc-gh-batur deleted the merging_from_main branch January 13, 2025 18:09
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants