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.

Sign up for GitHub

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

Jump to bottom

Added flag to allow override SnowflakeDialect div_is_floor_div flag #545

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Added flag to allow override SnowflakeDialect div_is_floor_div flag #545

wants to merge 4 commits into from

Conversation

sfc-gh-jmartinezramirez
Copy link

@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez commented Nov 19, 2024 •
edited
Loading

Please answer these questions before submitting your pull requests. Thanks!

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

    Fixes SNOW-1812881: Division broken #543

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding new credentials
    • I am adding a new dependency

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

    The default behavior of the flag div_is_floor_div was changed to False. This ensures that Python and Snowflake operators match the right division. In this first version a flag would be added force_div_is_floor_div to override the current behavior.

@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez requested a review from a team as a code owner November 19, 2024 21:22
@sfc-gh-dszmolka sfc-gh-dszmolka mentioned this pull request Nov 20, 2024
Open
@jochenott
Copy link

Looks good. You might want to enable these sqlalchemy-provided tests again, which I believe should pass now:
https://github.com/snowflakedb/snowflake-sqlalchemy/blob/main/tests/sqlalchemy_test_suite/test_suite_20.py#L84-L99

@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez requested a review from a team as a code owner November 21, 2024 21:37
jochenott
jochenott reviewed Nov 22, 2024
View reviewed changes
DESCRIPTION.md
- v1.7.0(November 22, 2024)
- (Unreleased)
- Added `force_div_is_floordiv` flag to override `div_is_floordiv` new default value `False` in `SnowflakeDialect`.
- With the flag in `False`, the `/` division operator will be treated as a float division and `//` as a floor division.
Copy link

@jochenott jochenott Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not how it works.

This flag is a signal from the dialect towards sqlalchemy that describes the behavior of this sql implementation (Snowflake). So if select 4/5 returns 0, div_is_floordiv must be set to True. If it returns 0.8, this flag must be set to False.

So the previous value of div_is_floordiv = True for snowflake was just a bug, which should be fixed. Just as for other bugs, there is no need to make this configurable.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jochenott

We agree that the correct value for the flag div_is_floordiv in Snowflake should be False. However, current users expect the / and // operators to work as true divisions, although / should behave as a true division and // as a floor division. That's why the behavioral change will be introduced as a flag, which will be removed in a future major release.

Thank you for pointing out this code issue and your contribution!!!

@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez force-pushed the SNOW-1812881-division branch 2 times, most recently from 24dc195 to 2d4f27c Compare November 22, 2024 15:04
@sfc-gh-jmartinezramirez
Added flag to allow customer to test new behavior of div_is_floordiv …
5578bd9 
…that will be introduce, using new flag force_div_floordiv allow to test the new division behavior.

Update sa14:scripts to ignore feature_v20 from execution
@sfc-gh-jmartinezramirez
Added warning for use of div_is_floor_div with True value.
85f5fd0 
Added tests to validate results of true and floor divisions using `force_div_is_floordiv` flag.
@sfc-gh-jmartinezramirez sfc-gh-jmartinezramirez changed the title Changed SnowflakeDialect flag div_is_floor_div to False Added flag to allow override SnowflakeDialect div_is_floor_div flag Dec 6, 2024
@jochenott
Copy link

Anything missing to get this merged and released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-jvasquezrojas sfc-gh-jvasquezrojas sfc-gh-jvasquezrojas left review comments

@jochenott jochenott jochenott left review comments

@sfc-gh-osalazarlizano sfc-gh-osalazarlizano sfc-gh-osalazarlizano approved these changes

@sfc-gh-ecuberojimenez sfc-gh-ecuberojimenez sfc-gh-ecuberojimenez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SNOW-1812881: Division broken
5 participants
@sfc-gh-jmartinezramirez @jochenott @sfc-gh-ecuberojimenez @sfc-gh-osalazarlizano @sfc-gh-jvasquezrojas