Skip to content

Conversation

@sfc-gh-helmeleegy
Copy link
Contributor

@sfc-gh-helmeleegy sfc-gh-helmeleegy commented Apr 8, 2025

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

    Fixes SNOW-2022698

  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
    • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support. Follow the link for more information: AST Support Guidelines
  3. Please describe how your code solves the related issue.

    Rename relaxed_ordering param into enforce_ordering per @sfc-gh-dolee's recommendation. The default value for the new param will be enforce_ordering=False which has the opposite meaning of the previous default value relaxed_ordering=False.
    Tests that were calling read_snowflake using the old default setting will switch to using the new default setting, which results in smaller query counts.

@sfc-gh-snowflakedb-snyk-sa
Copy link

sfc-gh-snowflakedb-snyk-sa commented Apr 8, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@sfc-gh-helmeleegy sfc-gh-helmeleegy force-pushed the helmeleegy-SNOW-2022698 branch from 8adc5f6 to 125f581 Compare April 8, 2025 18:05
@sfc-gh-helmeleegy sfc-gh-helmeleegy added the NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs label Apr 8, 2025
@sfc-gh-helmeleegy sfc-gh-helmeleegy changed the title SNOW-2022698: Rename relaxed_ordering param into enforce_ordering and update default value SNOW-2022698: Rename relaxed_ordering param into enforce_ordering and update default setting Apr 9, 2025
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.

This is mostly good, but I'm leaving comments that are mostly asking about cases where I think that we can't rely on the ordering from read_snowflake(), but we assume a certain order.

Some of these test cases seem to have the ordering issue even prior to this PR because we were already testing with relaxed_ordering=True, while others now only rely on a non-guaranteed ordering because they are using the new default of enforced_ordering=False. Could you please take on a JIRA ticket to fix 1) in another PR, and also address 2) in this PR? All of this assumes that I've correctly both sets of ordering issues :)

@sfc-gh-helmeleegy sfc-gh-helmeleegy force-pushed the helmeleegy-SNOW-2022698 branch from 9b8033c to 5ddd563 Compare April 11, 2025 20:38
@sfc-gh-helmeleegy sfc-gh-helmeleegy force-pushed the helmeleegy-SNOW-2022698 branch from 61c4e7b to 43ac3ec Compare April 12, 2025 09:19
@sfc-gh-helmeleegy
Copy link
Contributor Author

This is mostly good, but I'm leaving comments that are mostly asking about cases where I think that we can't rely on the ordering from read_snowflake(), but we assume a certain order.

Some of these test cases seem to have the ordering issue even prior to this PR because we were already testing with relaxed_ordering=True, while others now only rely on a non-guaranteed ordering because they are using the new default of enforced_ordering=False. Could you please take on a JIRA ticket to fix 1) in another PR, and also address 2) in this PR? All of this assumes that I've correctly both sets of ordering issues :)

Fair point. Calls using the new default are now followed by an explicit sort step to make sure that we avoid test flakiness.

@sfc-gh-yuwang
Copy link
Collaborator

the changes looks good to me, but I want to ask to run our daily precommit test/pandas precommit test on this PR to see if it has any side effects since this is such a big PR

@sfc-gh-helmeleegy
Copy link
Contributor Author

sfc-gh-helmeleegy commented Apr 15, 2025

the changes looks good to me, but I want to ask to run our daily precommit test/pandas precommit test on this PR to see if it has any side effects since this is such a big PR

@sfc-gh-yuwang Sure. Here are the two jobs. They both look good:
https://github.com/snowflakedb/snowpark-python/actions/runs/14458947108
https://github.com/snowflakedb/snowpark-python/actions/runs/14458960726

@sfc-gh-helmeleegy sfc-gh-helmeleegy merged commit ed80a58 into main Apr 15, 2025
114 of 156 checks passed
@sfc-gh-helmeleegy sfc-gh-helmeleegy deleted the helmeleegy-SNOW-2022698 branch April 15, 2025 17:19
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2025
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.

5 participants