Skip to content

SNOW-3570860 move to sa 20#690

Open
sfc-gh-mraba wants to merge 13 commits into
mainfrom
SNOW-3570860-move-to-sa-20
Open

SNOW-3570860 move to sa 20#690
sfc-gh-mraba wants to merge 13 commits into
mainfrom
SNOW-3570860-move-to-sa-20

Conversation

@sfc-gh-mraba

@sfc-gh-mraba sfc-gh-mraba commented May 25, 2026

Copy link
Copy Markdown
Collaborator

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-3570860

  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.

  • Drop SQLAlchemy 1.4 compatibility code and require SQLAlchemy 2.0+.
  • Update supported Python versions to 3.9 through 3.14.
  • Remove obsolete SQLAlchemy 1.4 test paths, CI matrix entries, and compatibility helpers.
  • Keep UUID reflection available on SQLAlchemy 2.0 and return UUID values as strings.
  • Clean up SQLAlchemy 2-only reflection and ORM compiler paths.
  • Move unreleased changelog entries under the v2.0.0 section.

@sfc-gh-mraba sfc-gh-mraba self-assigned this May 26, 2026
@sfc-gh-mraba sfc-gh-mraba changed the title Snow 3570860 move to sa 20 SNOW-3570860 move to sa 20 May 27, 2026
Comment thread .github/workflows/build_test.yml
isolation_level: Optional[str] = SnowflakeIsolationLevel.READ_COMMITTED.value,
enable_decfloat: bool = False,
case_sensitive_identifiers: bool = False,
cache_column_metadata: bool = False,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Q (for potential future PRs): Do we have other flags that could be removed in favor of better defaults?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is open for discussions before RC release.

Comment thread src/snowflake/sqlalchemy/snowdialect.py

class TrueDivTest(_TrueDivTest):
@pytest.mark.skip("`//` not supported")
def test_floordiv_integer_bound(self, connection):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Q: Just so I understand correctly, we're skipping those as we have our own tests that work with the dialect-level flag, so we don't need the tests provided by SQA for truediv, correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch — the skip reason is incorrect. SA compiles // to FLOOR(x / y), not a SQL // operator, so "Snowflake does not provide //" is wrong. Snowflake supports FLOOR() fine.

The real issue is force_div_is_floordiv=True (the current default):

  • truediv / tests expect 15/10 = 1.5 — these pass because visit_truediv_binary delegates to SA's base which emits 15 / CAST(10 AS NUMERIC) = 1.5
  • floordiv // tests expect 15//10 = 1 — these would also pass via FLOOR(15 / 10) = 1, just with a PendingDeprecationWarning

So the skips are likely unnecessary. However, this is tied to the open force_div_is_floordiv discussion — if we flip its default to False in v2.0.0 (which I'm leaning towards given we're doing a major version bump), the warning disappears and the entire TrueDivTest subclass can be dropped. Leaving this pending until Q2 is resolved.

@sfc-gh-mraba sfc-gh-mraba force-pushed the SNOW-3570860-move-to-sa-20 branch from c322bec to 55fe4ae Compare June 2, 2026 09:01
@sfc-gh-mraba sfc-gh-mraba marked this pull request as ready for review June 8, 2026 08:59
@sfc-gh-mraba sfc-gh-mraba requested a review from a team as a code owner June 8, 2026 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants