-
Notifications
You must be signed in to change notification settings - Fork 152
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
Changed SnowflakeDialect flag div_is_floor_div to False #545
base: main
Are you sure you want to change the base?
Conversation
Looks good. You might want to enable these sqlalchemy-provided tests again, which I believe should pass now: |
719b39b
to
cfc4f11
Compare
cfc4f11
to
a86f140
Compare
- 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!!!
…division operator as floor div. Changed flag div_is_floor_div to False.
24dc195
to
2d4f27c
Compare
…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
2d4f27c
to
5578bd9
Compare
Added tests to validate results of true and floor divisions using `force_div_is_floordiv` flag.
d187421
to
85f5fd0
Compare
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1812881: Division broken #543
Fill out the following pre-review checklist:
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.