From da3ba8da224a63fba06743b58dd91a30c08d7df6 Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Tue, 19 Nov 2024 15:18:44 -0600 Subject: [PATCH 1/4] Changed default behavior of SnowflakeDialect to disable the use of / division operator as floor div. Changed flag div_is_floor_div to False. --- src/snowflake/sqlalchemy/snowdialect.py | 3 +++ tests/test_compiler.py | 30 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/snowflake/sqlalchemy/snowdialect.py b/src/snowflake/sqlalchemy/snowdialect.py index 6f1493c3..58116910 100644 --- a/src/snowflake/sqlalchemy/snowdialect.py +++ b/src/snowflake/sqlalchemy/snowdialect.py @@ -79,6 +79,9 @@ class SnowflakeDialect(default.DefaultDialect): colspecs = colspecs ischema_names = ischema_names + # target database treats the / division operator as “floor division” + div_is_floordiv = False + # all str types must be converted in Unicode convert_unicode = True diff --git a/tests/test_compiler.py b/tests/test_compiler.py index 55451c2f..8698a8d5 100644 --- a/tests/test_compiler.py +++ b/tests/test_compiler.py @@ -135,3 +135,33 @@ def test_outer_lateral_join(): str(stmt.compile(dialect=snowdialect.dialect())) == "SELECT colname AS label \nFROM abc JOIN LATERAL flatten(PARSE_JSON(colname2)) AS anon_1 GROUP BY colname" ) + + +def test_division_operator(): + col1 = column("col1") + col2 = column("col2") + stmt = col1 / col2 + assert str(stmt.compile(dialect=snowdialect.dialect())) == "col1 / col2" + + +def test_division_operator_with_denominator_expr(): + col1 = column("col1") + col2 = column("col2") + stmt = col1 / func.sqrt(col2) + assert str(stmt.compile(dialect=snowdialect.dialect())) == "col1 / sqrt(col2)" + + +def test_floor_division_operator(): + col1 = column("col1") + col2 = column("col2") + stmt = col1 // col2 + assert str(stmt.compile(dialect=snowdialect.dialect())) == "FLOOR(col1 / col2)" + + +def test_floor_division_operator_with_denominator_expr(): + col1 = column("col1") + col2 = column("col2") + stmt = col1 // func.sqrt(col2) + assert ( + str(stmt.compile(dialect=snowdialect.dialect())) == "FLOOR(col1 / sqrt(col2))" + ) From 9283bc53ab52c485ca9788258dd12f65b696cbaa Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Tue, 19 Nov 2024 15:26:03 -0600 Subject: [PATCH 2/4] Update Description.md --- DESCRIPTION.md | 1 + 1 file changed, 1 insertion(+) diff --git a/DESCRIPTION.md b/DESCRIPTION.md index 1dab72f1..147b1591 100644 --- a/DESCRIPTION.md +++ b/DESCRIPTION.md @@ -10,6 +10,7 @@ Source code is also available at: # Release Notes - (Unreleased) - Fix return value of snowflake get_table_names + - Fix flag `div_is_floordiv` to `False` in `SnowflakeDialect` to avoid division wrong CAST when trying to do a integer division when using `/` - v1.7.2(December 18, 2024) - Fix quoting of `_` as column name From 5e4e5191f39a8265b869b732714be3ba7a417072 Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Tue, 19 Nov 2024 18:10:20 -0600 Subject: [PATCH 3/4] Added flag to allow customer to test new behavior of div_is_floordiv 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 --- DESCRIPTION.md | 5 +- pyproject.toml | 6 ++ src/snowflake/sqlalchemy/snowdialect.py | 8 +++ tests/conftest.py | 20 ++++++ tests/test_compiler.py | 82 ++++++++++++++++++++----- 5 files changed, 104 insertions(+), 17 deletions(-) diff --git a/DESCRIPTION.md b/DESCRIPTION.md index 147b1591..00f860ce 100644 --- a/DESCRIPTION.md +++ b/DESCRIPTION.md @@ -10,7 +10,10 @@ Source code is also available at: # Release Notes - (Unreleased) - Fix return value of snowflake get_table_names - - Fix flag `div_is_floordiv` to `False` in `SnowflakeDialect` to avoid division wrong CAST when trying to do a integer division when using `/` + - 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. + - This flag is added to maintain backward compatibility with the previous behavior of Snowflake Dialect division. + - This flag will be removed in the future and Snowflake Dialect will use `div_is_floor_div` as `False`. - v1.7.2(December 18, 2024) - Fix quoting of `_` as column name diff --git a/pyproject.toml b/pyproject.toml index b0ae04c4..b22dc293 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -87,6 +87,11 @@ extra-dependencies = ["SQLAlchemy>=1.4.19,<2.0.0"] features = ["development", "pandas"] python = "3.8" +[tool.hatch.envs.sa14.scripts] +test-dialect = "pytest --ignore_v20_test -ra -vvv --tb=short --cov snowflake.sqlalchemy --cov-append --junitxml ./junit.xml --ignore=tests/sqlalchemy_test_suite tests/" +test-dialect-compatibility = "pytest --ignore_v20_test -ra -vvv --tb=short --cov snowflake.sqlalchemy --cov-append --junitxml ./junit.xml tests/sqlalchemy_test_suite" +test-dialect-aws = "pytest --ignore_v20_test -m \"aws\" -ra -vvv --tb=short --cov snowflake.sqlalchemy --cov-append --junitxml ./junit.xml --ignore=tests/sqlalchemy_test_suite tests/" + [tool.hatch.envs.default.env-vars] COVERAGE_FILE = "coverage.xml" SQLACHEMY_WARN_20 = "1" @@ -134,4 +139,5 @@ markers = [ "requires_external_volume: tests that needs a external volume to be executed", "external: tests that could but should only run on our external CI", "feature_max_lob_size: tests that could but should only run on our external CI", + "feature_v20: tests that could but should only run on SqlAlchemy v20", ] diff --git a/src/snowflake/sqlalchemy/snowdialect.py b/src/snowflake/sqlalchemy/snowdialect.py index 58116910..8fac73bc 100644 --- a/src/snowflake/sqlalchemy/snowdialect.py +++ b/src/snowflake/sqlalchemy/snowdialect.py @@ -148,12 +148,20 @@ class SnowflakeDialect(default.DefaultDialect): supports_identity_columns = True + def __init__( self, + force_div_is_floordiv: bool = True, isolation_level: Optional[str] = SnowflakeIsolationLevel.READ_COMMITTED.value, **kwargs: Any, ): super().__init__(isolation_level=isolation_level, **kwargs) + self.force_div_is_floordiv = force_div_is_floordiv + self.div_is_floordiv = force_div_is_floordiv + + def initialize(self, connection): + super().initialize(connection) + self.div_is_floordiv = self.force_div_is_floordiv @classmethod def dbapi(cls): diff --git a/tests/conftest.py b/tests/conftest.py index f2045121..5e0fd3ed 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -47,6 +47,26 @@ TEST_SCHEMA = f"sqlalchemy_tests_{str(uuid.uuid4()).replace('-', '_')}" +def pytest_addoption(parser): + parser.addoption( + "--ignore_v20_test", + action="store_true", + default=False, + help="skip sqlalchemy 2.0 exclusive tests", + ) + + +def pytest_collection_modifyitems(config, items): + if config.getoption("--ignore_v20_test"): + # --ignore_v20_test given in cli: skip sqlalchemy 2.0 tests + skip_feature_v2 = pytest.mark.skip( + reason="need remove --ignore_v20_test option to run" + ) + for item in items: + if "feature_v20" in item.keywords: + item.add_marker(skip_feature_v2) + + @pytest.fixture(scope="session") def on_travis(): return os.getenv("TRAVIS", "").lower() == "true" diff --git a/tests/test_compiler.py b/tests/test_compiler.py index 8698a8d5..72174e55 100644 --- a/tests/test_compiler.py +++ b/tests/test_compiler.py @@ -2,12 +2,14 @@ # Copyright (c) 2012-2023 Snowflake Computing Inc. All rights reserved. # +import pytest from sqlalchemy import Integer, String, and_, func, insert, select from sqlalchemy.schema import DropColumnComment, DropTableComment from sqlalchemy.sql import column, quoted_name, table from sqlalchemy.testing.assertions import AssertsCompiledSQL from snowflake.sqlalchemy import snowdialect +from src.snowflake.sqlalchemy.snowdialect import SnowflakeDialect table1 = table( "table1", column("id", Integer), column("name", String), column("value", Integer) @@ -137,31 +139,79 @@ def test_outer_lateral_join(): ) -def test_division_operator(): - col1 = column("col1") - col2 = column("col2") +def test_division_operator_with_force_div_is_floordiv_false(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) stmt = col1 / col2 - assert str(stmt.compile(dialect=snowdialect.dialect())) == "col1 / col2" + assert ( + str(stmt.compile(dialect=SnowflakeDialect(force_div_is_floordiv=False))) + == "col1 / col2" + ) + + +def test_division_operator_with_denominator_expr_force_div_is_floordiv_false(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) + stmt = col1 / func.sqrt(col2) + assert ( + str(stmt.compile(dialect=SnowflakeDialect(force_div_is_floordiv=False))) + == "col1 / sqrt(col2)" + ) + + +def test_division_operator_with_force_div_is_floordiv_default_true(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) + stmt = col1 / col2 + assert ( + str(stmt.compile(dialect=SnowflakeDialect())) == "col1 / CAST(col2 AS NUMERIC)" + ) -def test_division_operator_with_denominator_expr(): - col1 = column("col1") - col2 = column("col2") +def test_division_operator_with_denominator_expr_force_div_is_floordiv_default_true(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) stmt = col1 / func.sqrt(col2) - assert str(stmt.compile(dialect=snowdialect.dialect())) == "col1 / sqrt(col2)" + assert ( + str(stmt.compile(dialect=SnowflakeDialect())) + == "col1 / CAST(sqrt(col2) AS NUMERIC)" + ) -def test_floor_division_operator(): - col1 = column("col1") - col2 = column("col2") +@pytest.mark.feature_v20 +def test_floor_division_operator_force_div_is_floordiv_false(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) stmt = col1 // col2 - assert str(stmt.compile(dialect=snowdialect.dialect())) == "FLOOR(col1 / col2)" + assert ( + str(stmt.compile(dialect=SnowflakeDialect(force_div_is_floordiv=False))) + == "FLOOR(col1 / col2)" + ) -def test_floor_division_operator_with_denominator_expr(): - col1 = column("col1") - col2 = column("col2") +@pytest.mark.feature_v20 +def test_floor_division_operator_with_denominator_expr_force_div_is_floordiv_false(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) stmt = col1 // func.sqrt(col2) assert ( - str(stmt.compile(dialect=snowdialect.dialect())) == "FLOOR(col1 / sqrt(col2))" + str(stmt.compile(dialect=SnowflakeDialect(force_div_is_floordiv=False))) + == "FLOOR(col1 / sqrt(col2))" ) + + +@pytest.mark.feature_v20 +def test_floor_division_operator_force_div_is_floordiv_default_true(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) + stmt = col1 // col2 + assert str(stmt.compile(dialect=SnowflakeDialect())) == "col1 / col2" + + +@pytest.mark.feature_v20 +def test_floor_division_operator_with_denominator_expr_force_div_is_floordiv_default_true(): + col1 = column("col1", Integer) + col2 = column("col2", Integer) + stmt = col1 // func.sqrt(col2) + res = stmt.compile(dialect=SnowflakeDialect()) + assert str(res) == "FLOOR(col1 / sqrt(col2))" From 24b4bb0a5ed36b14bd5896969c33e476774569d7 Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Fri, 22 Nov 2024 18:08:55 -0600 Subject: [PATCH 4/4] Added warning for use of div_is_floor_div with `True` value. Added tests to validate results of true and floor divisions using `force_div_is_floordiv` flag. --- src/snowflake/sqlalchemy/base.py | 21 ++++++ src/snowflake/sqlalchemy/snowdialect.py | 1 - tests/test_compiler.py | 13 ++-- tests/test_core.py | 87 ++++++++++++++++++++++++- 4 files changed, 113 insertions(+), 9 deletions(-) diff --git a/src/snowflake/sqlalchemy/base.py b/src/snowflake/sqlalchemy/base.py index 9ce8b83c..0226d37d 100644 --- a/src/snowflake/sqlalchemy/base.py +++ b/src/snowflake/sqlalchemy/base.py @@ -6,6 +6,7 @@ import operator import re import string +import warnings from typing import List from sqlalchemy import exc as sa_exc @@ -802,6 +803,26 @@ def visit_join(self, join, asfrom=False, from_linter=None, **kwargs): + join.onclause._compiler_dispatch(self, from_linter=from_linter, **kwargs) ) + def visit_truediv_binary(self, binary, operator, **kw): + if self.dialect.div_is_floordiv: + warnings.warn( + "div_is_floordiv value will be changed to False in a future release. This will generate a behavior change on true and floor division. Please review https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#python-division-operator-performs-true-division-for-all-backends-added-floor-division", + PendingDeprecationWarning, + stacklevel=2, + ) + return ( + self.process(binary.left, **kw) + " / " + self.process(binary.right, **kw) + ) + + def visit_floordiv_binary(self, binary, operator, **kw): + if self.dialect.div_is_floordiv and IS_VERSION_20: + warnings.warn( + "div_is_floordiv value will be changed to False in a future release. This will generate a behavior change on true and floor division. Please review https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#python-division-operator-performs-true-division-for-all-backends-added-floor-division", + PendingDeprecationWarning, + stacklevel=2, + ) + return super().visit_floordiv_binary(binary, operator, **kw) + def render_literal_value(self, value, type_): # escape backslash return super().render_literal_value(value, type_).replace("\\", "\\\\") diff --git a/src/snowflake/sqlalchemy/snowdialect.py b/src/snowflake/sqlalchemy/snowdialect.py index 8fac73bc..bba1160f 100644 --- a/src/snowflake/sqlalchemy/snowdialect.py +++ b/src/snowflake/sqlalchemy/snowdialect.py @@ -148,7 +148,6 @@ class SnowflakeDialect(default.DefaultDialect): supports_identity_columns = True - def __init__( self, force_div_is_floordiv: bool = True, diff --git a/tests/test_compiler.py b/tests/test_compiler.py index 72174e55..0eea4607 100644 --- a/tests/test_compiler.py +++ b/tests/test_compiler.py @@ -139,6 +139,7 @@ def test_outer_lateral_join(): ) +@pytest.mark.feature_v20 def test_division_operator_with_force_div_is_floordiv_false(): col1 = column("col1", Integer) col2 = column("col2", Integer) @@ -149,6 +150,7 @@ def test_division_operator_with_force_div_is_floordiv_false(): ) +@pytest.mark.feature_v20 def test_division_operator_with_denominator_expr_force_div_is_floordiv_false(): col1 = column("col1", Integer) col2 = column("col2", Integer) @@ -159,23 +161,20 @@ def test_division_operator_with_denominator_expr_force_div_is_floordiv_false(): ) +@pytest.mark.feature_v20 def test_division_operator_with_force_div_is_floordiv_default_true(): col1 = column("col1", Integer) col2 = column("col2", Integer) stmt = col1 / col2 - assert ( - str(stmt.compile(dialect=SnowflakeDialect())) == "col1 / CAST(col2 AS NUMERIC)" - ) + assert str(stmt.compile(dialect=SnowflakeDialect())) == "col1 / col2" +@pytest.mark.feature_v20 def test_division_operator_with_denominator_expr_force_div_is_floordiv_default_true(): col1 = column("col1", Integer) col2 = column("col2", Integer) stmt = col1 / func.sqrt(col2) - assert ( - str(stmt.compile(dialect=SnowflakeDialect())) - == "col1 / CAST(sqrt(col2) AS NUMERIC)" - ) + assert str(stmt.compile(dialect=SnowflakeDialect())) == "col1 / sqrt(col2)" @pytest.mark.feature_v20 diff --git a/tests/test_core.py b/tests/test_core.py index dfe0f714..a25342ac 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -31,13 +31,15 @@ create_engine, dialects, exc, + func, insert, inspect, text, ) from sqlalchemy.exc import DBAPIError, NoSuchTableError, OperationalError -from sqlalchemy.sql import and_, not_, or_, select +from sqlalchemy.sql import and_, literal, not_, or_, select from sqlalchemy.sql.ddl import CreateTable +from sqlalchemy.testing.assertions import eq_ import snowflake.connector.errors import snowflake.sqlalchemy.snowdialect @@ -1864,3 +1866,86 @@ def test_snowflake_sqlalchemy_as_valid_client_type(): snowflake.connector.connection.DEFAULT_CONFIGURATION[ "internal_application_version" ] = origin_internal_app_version + + +@pytest.mark.parametrize( + "operation", + [ + [ + literal(5), + literal(10), + 0.5, + ], + [literal(5), func.sqrt(literal(10)), 1.5811388300841895], + [literal(4), literal(5), decimal.Decimal("0.800000")], + [literal(2), literal(2), 1.0], + [literal(3), literal(2), 1.5], + [literal(4), literal(1.5), 2.666667], + [literal(5.5), literal(10.7), 0.5140187], + [literal(5.5), literal(8), 0.6875], + ], +) +def test_true_division_operation(engine_testaccount, operation): + # expected_warning = "div_is_floordiv value will be changed to False in a future release. This will generate a behavior change on true and floor division. Please review https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#python-division-operator-performs-true-division-for-all-backends-added-floor-division" + # with pytest.warns(PendingDeprecationWarning, match=expected_warning): + with engine_testaccount.connect() as conn: + eq_( + conn.execute(select(operation[0] / operation[1])).fetchall(), + [((operation[2]),)], + ) + + +@pytest.mark.parametrize( + "operation", + [ + [literal(5), literal(10), 0.5, 0.5], + [literal(5), func.sqrt(literal(10)), 1.5811388300841895, 1.0], + [ + literal(4), + literal(5), + decimal.Decimal("0.800000"), + decimal.Decimal("0.800000"), + ], + [literal(2), literal(2), 1.0, 1.0], + [literal(3), literal(2), 1.5, 1.5], + [literal(4), literal(1.5), 2.666667, 2.0], + [literal(5.5), literal(10.7), 0.5140187, 0], + [literal(5.5), literal(8), 0.6875, 0.6875], + ], +) +@pytest.mark.feature_v20 +def test_division_force_div_is_floordiv_default(engine_testaccount, operation): + expected_warning = "div_is_floordiv value will be changed to False in a future release. This will generate a behavior change on true and floor division. Please review https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#python-division-operator-performs-true-division-for-all-backends-added-floor-division" + with pytest.warns(PendingDeprecationWarning, match=expected_warning): + with engine_testaccount.connect() as conn: + eq_( + conn.execute( + select(operation[0] / operation[1], operation[0] // operation[1]) + ).fetchall(), + [(operation[2], operation[3])], + ) + + +@pytest.mark.parametrize( + "operation", + [ + [literal(5), literal(10), 0.5, 0], + [literal(5), func.sqrt(literal(10)), 1.5811388300841895, 1.0], + [literal(4), literal(5), decimal.Decimal("0.800000"), 0], + [literal(2), literal(2), 1.0, 1.0], + [literal(3), literal(2), 1.5, 1], + [literal(4), literal(1.5), 2.666667, 2.0], + [literal(5.5), literal(10.7), 0.5140187, 0], + [literal(5.5), literal(8), 0.6875, 0], + ], +) +@pytest.mark.feature_v20 +def test_division_force_div_is_floordiv_false(db_parameters, operation): + engine = create_engine(URL(**db_parameters), **{"force_div_is_floordiv": False}) + with engine.connect() as conn: + eq_( + conn.execute( + select(operation[0] / operation[1], operation[0] // operation[1]) + ).fetchall(), + [(operation[2], operation[3])], + )