-
Notifications
You must be signed in to change notification settings - Fork 153
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
Add basic compatibility with SQLAlchemy 2.0 #414
Conversation
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.
Explanations of the changes:
src/snowflake/sqlalchemy/base.py
Outdated
@@ -296,7 +295,7 @@ def visit_aws_bucket(self, aws_bucket, **kw): | |||
encryption_list.sort(key=operator.itemgetter(0)) | |||
encryption = "ENCRYPTION=({})".format( | |||
" ".join( | |||
("{}='{}'" if isinstance(v, string_types) else "{}={}").format(n, v) | |||
("{}='{}'" if isinstance(v, str) else "{}={}").format(n, v) |
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.
string_types
no longer exists in SQLAlchemy 2.0. It used to be for Python 2 compatibility, but even in SQLAlchemy 1.4 it's just string_types = (str,)
.
- array_type: not supported in snowflake | ||
- uuid_data_type: not supported in snowflake |
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.
These are new properties (or at least new ones checked in the sqlalchemy test suite) that were added in 2.0. Since 1.4 simply doesn't check them, the addition is backwards-compatible.
# no longer used for SQLAlchemy 2.0, so any value works | ||
# String.RETURNS_UNICODE for backwards compatibility with 1.4 | ||
returns_unicode_strings = ( | ||
String.RETURNS_UNICODE if hasattr(String, "RETURNS_UNICODE") else None | ||
) |
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 necessary in 2.0, but kept for backwards-compatibility. String.RETURNS_UNICODE
doesn't exist in 2.0.
# Remove once support for sqlalchemy 1.4 is dropped | ||
@classmethod | ||
def dbapi(cls): | ||
return cls.import_dbapi() | ||
|
||
@classmethod | ||
def import_dbapi(cls): |
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.
Not a strictly necessary change, but produces a deprecation warning in 2.0.
def has_table(self, connection, table_name, schema=None, **kw): | ||
""" | ||
Checks if the table exists | ||
""" | ||
return self._has_object(connection, "TABLE", table_name, schema) | ||
|
||
def has_sequence(self, connection, sequence_name, schema=None): | ||
def has_sequence(self, connection, sequence_name, schema=None, **kw): |
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.
These functions get passed a new argument info_cache
. Not certain what it is for, but kwargs makes this change backwards-compatible and we can keep the old implementation for now.
tests/test_core.py
Outdated
@@ -423,7 +422,7 @@ def test_table_does_not_exist(engine_testaccount): | |||
""" | |||
meta = MetaData() | |||
with pytest.raises(NoSuchTableError): | |||
Table("does_not_exist", meta, autoload=True, autoload_with=engine_testaccount) | |||
Table("does_not_exist", meta, autoload_with=engine_testaccount) |
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.
autoload
is no longer a valid argument in 2.0. Code works fine without it in 1.4 as well.
tests/test_core.py
Outdated
original_execute = inspector.bind.execute | ||
|
||
def mock_helper(command, *args, **kwargs): | ||
if "_get_schema_columns" in command: | ||
# Creating exception exactly how SQLAlchemy does | ||
raise DBAPIError.instance( | ||
""" | ||
SELECT /* sqlalchemy:_get_schema_columns */ | ||
ic.table_name, | ||
ic.column_name, | ||
ic.data_type, | ||
ic.character_maximum_length, | ||
ic.numeric_precision, | ||
ic.numeric_scale, | ||
ic.is_nullable, | ||
ic.column_default, | ||
ic.is_identity, | ||
ic.comment | ||
FROM information_schema.columns ic | ||
WHERE ic.table_schema='schema_name' | ||
ORDER BY ic.ordinal_position""", | ||
{"table_schema": "TESTSCHEMA"}, | ||
ProgrammingError( | ||
"Information schema query returned too much data. Please repeat query with more " | ||
"selective predicates.", | ||
90030, | ||
), | ||
Error, | ||
hide_parameters=False, | ||
connection_invalidated=False, | ||
dialect=SnowflakeDialect(), | ||
ismulti=None, | ||
) | ||
else: | ||
return original_execute(command, *args, **kwargs) | ||
|
||
with patch.object(inspector.bind, "execute", side_effect=mock_helper): |
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 mock was actually unused in the test, as engine.execute
was never called. Since engine.execute
no longer exists in 2.0, I removed it.
tests/test_core.py
Outdated
columns = table_reflected.columns | ||
assert ( | ||
len(columns) == len(ischema_names_baseline) - 1 | ||
) # -1 because FIXED is not supported | ||
|
||
|
||
def test_result_type_and_value(engine_testaccount): | ||
with engine_testaccount.connect() as conn: | ||
with engine_testaccount.begin() as conn: |
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.
Not starting a transaction explicitly was causing warnings. The code works in both versions still after this change.
tox.ini
Outdated
@@ -114,6 +114,7 @@ markers = | |||
timeout: tests that need a timeout time | |||
internal: tests that could but should only run on our internal CI | |||
external: tests that could but should only run on our external CI | |||
backend: used in the sqlalchemy 2.0 test suite |
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.
SQLAlchemy 2.0 test suite adds a marker and since --strict-markers
is set, pytest will refuse to run the tests without it added in the config. Doesn't affect 1.4.
Should this restriction be removed in this PR? snowflake-sqlalchemy/setup.cfg Line 50 in e3f675e
|
I'm unsure yet; the test failures make it seem that it wouldn't be fully compatible with 2.0 yet, though it will probably work for most use-cases. |
Also if someone could approve the CI run, I'm interested whether my local setup behaves the same as the CI and whether the same tests fail. |
Waiting to be merged ! |
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 (partially) SNOW-739749: snowflake-sqlalchemy 1.4.4 requires sqlalchemy<2.0.0,>=1.4.0, but you have sqlalchemy 2.0.1 which is incompatible. #380
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Updates code which is incompatible with SQLAlchemy 2.0. See comments for why specific changes were done.
pytest
all tests succeeded.pytest tests/sqlalchemy_test_suite
all tests succeeded.pytest --run_v20_sqlalchemy
all tests succeeded.pytest --run_v20_sqlalchemy tests/sqlalchemy_test_suite
row count tests failed:pytest --run_v20_sqlalchemy
(can't run without the flag, will error out) no tests failed.pytest --run_v20_sqlalchemy tests/sqlalchemy_test_suite
row count, date format, integer and some reflection caching tests failed (I assume newly added in 2.0):Not sure what to do about the failed tests - the row count ones failed during cleanup for me, so may work fine in CI. Datetime and integer-based tests are a bit more complicated as snowflake-sqlalchemy returns strings for datetimes and integer math works differently than decimal. The reflection caching could probably be fixed if I could figure out how it works 😅