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.

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

Create RedshiftDialectMixin class. Add Psycopg2CFFIRedshiftDialect #231

Merged
merged 21 commits into from
Sep 9, 2021

Conversation

Brooke-white
Copy link
Contributor

Builds upon graingert’s work in #100 to refactor dialect code to utilize a RedshiftDialectMixin class to define driver specific dialects.

Additionally, the PyscopgRedshiftDialectMixin class is defined and used as a base class for psycopg flavored dialects. It holds the implementation for the create_connect_args() method, which previously lived in theRedshiftDialect class. My thoughts here are that this class could also be utilized in supporting a psycopg3 dialect in the future.

While graingert’s work in #100 added support for additional dialects (pg8000, pypostgresql, zxjdbc), I believe supporting these drivers would require some additional effort. Support for these drivers is not included in this PR.

Tests are parameterized using pytest.fixture to run on each defined dialect (i.e. redshift, redshift+psycopg2, redshift+psycopg2cffi). Unit tests utilize a parameterized ”stub” redshift dialect pytest.fixture to avoid needing an Amazon Redshift cluster. Unsure if we want to run all tests using both redshift and redshift+psycopg2, as they are using the same dialect — so I’ll leave this up to the reviewers :)

All tests (including ones requiring a Redshift cluster) have been run with nominal results.

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

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

From a first read-through, this looks pretty comprehensive. I'd like to see some documentation about the existence of the CFFI variant, and I left some comments about naming. I wish I were better acquainted with modern sqlalchemy conventions to make stronger suggestions on naming.

Perhaps @zzzeek would be willing to do a quick drive-by about how the class naming and registration looks here.

registry.register(
"redshift.psycopg2", "sqlalchemy_redshift.dialect", "RedshiftDialect"
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any idea whether removing RedshiftDialect could affect existing user code? It's not clear to me where this name would get referenced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For users directly using RedshiftDialect class, this would be a breaking change. Searching GitHub, I found a number of projects reference this class directly who would impacted. see here. So we probably want to take a different approach here.

One option to avoid a breaking change here could be to re-add RedshiftDialect having it inherit from Pyscopg2RedshiftDialect. Or we simply re-name Pyscopg2RedshiftDialect -> RedshiftDialect.

Copy link
Member

Choose a reason for hiding this comment

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

I do like modernizing the names here, so I think it's good to add some shim for compatibility rather than staying stuck with the old name.

I think we can get away with something like this in dialect.py:

# Add RedshiftDialect synonym for backwards compatibility.
RedshiftDialect = RedshiftDialect_psycopg2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in ae43dfa

@@ -882,6 +866,49 @@ def _get_all_constraint_info(self, connection, **kw):
return all_constraints


class PsycopgRedshiftDialectMixin(RedshiftDialectMixin):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class PsycopgRedshiftDialectMixin(RedshiftDialectMixin):
class Psycopg2RedshiftDialectMixin(RedshiftDialectMixin):

I assume that the missing "2" here is a typo, but let me know if there's additional nuance here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch -- will fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 96b9af0

@@ -882,6 +866,49 @@ def _get_all_constraint_info(self, connection, **kw):
return all_constraints


class PsycopgRedshiftDialectMixin(RedshiftDialectMixin):
"""
Define Psycopg specific behavior.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Define Psycopg specific behavior.
Define behavior specific to ``psycopg2``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 17c6a6b

Comment on lines 11 to 12
from sqlalchemy.dialects.postgresql.psycopg2 import PGDialect_psycopg2
from sqlalchemy.dialects.postgresql import (
psycopg2, psycopg2cffi
Copy link
Member

Choose a reason for hiding this comment

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

Are PGDialect_psycopg2 and psycopg2 synonyms? Do we know why the longer name exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are PGDialect_psycopg2 and psycopg2 synonyms?

From Sqlalchemy source PGDialect_psycopg2,sqlalchemy.dialects.postrgresql.psycopg2.dialect, and sqlalchemy-dialects.postgres.__init__.py, I believe so.

Do we know why the longer name exists?

The name still exists. There is an equivelent name for PGDialect_psycopg2cffi. I can modify this import to be the following if it's preferred:

from sqlalchemy.dialects.postgresql.psycopg2 import PGDialect_psycopg2
from sqlalchemy.dialects.postgresql.psycopg2cffi import PGDialect_psycopg2cffi

Copy link
Member

Choose a reason for hiding this comment

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

I see now that you inherit from psycopg2.dialect later on, and indeed that's a synonym of PGDialect_psycopg2.

I would prefer, though, that we use the class names. It feels more natural for expressing class inheritance. And there will be some more obvious symmetry, too, with our own class names assuming we update to use RedshiftDialect_psycopg2, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 291d88e

return cargs, default_args


class Psycopg2RedshiftDialect(
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to following existing naming conventions in SQLAlchemy where possible. From the postgresql dialect code, it looks like this would be called RedshiftDialect_psycopg2. Does that seem like a reasonable choice here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sounds good. I will make this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 29d5704

@zzzeek
Copy link

zzzeek commented Aug 30, 2021

I havent looked at everything but what stands out to me is the test suite refactoring. not sure how feasible this is but assuming redshift is using SQLAlchemy's test harness, we dont generally have dialect names hardcoded in our test suite as this would not scale, and the commandline runner can automatically run the tests against any number of dialects. instead of targeting "dialect = XDialect()" everywhere you instead look at the global "sqlalchemy.testing.config.db.dialect" for the current dialect. Directives associated with test classes like __backend__ = True will automatically run class-based test suites for every "dialect" currently selected.

our commandline to run the tests for postgresql looks like:

tox -e py3-postgresql

which will then run a pytest like this:

python -m pytest --db postgresql --dbdriver psycopg2 --dbdriver asyncpg --dbdriver pg8000

the suite will generate URLs given each driver and run the test suite against all the above DBAPIs.

Just something to think about as sqlalchemy-redshift is definitely going to want to support other DBAPIs, at least one of asyncpg or psycopg3 for async support, for example.

Copy link
Contributor Author

@Brooke-white Brooke-white left a comment

Choose a reason for hiding this comment

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

thank you both for the feedback!

As for documentation regarding the CFFI variant, from what I saw in the sqlalchemy source, PGDialect_psycopg2cffi seems very similar to PGDialect_psycopg2, save for some methods having logic around extras/extensions and the package's versioning.

It looks like @zzzeek reviewed a PR associated with PGDialect_psycopg2cffi, sqlalchemy #3052. I see some mentions of differences in unicode bind parameter names and floating point values, but I am far from knowledgable on this subject and this PR is from quite a few years ago, so things have probably changed.

Regarding the test suite, I don't believe sqlalchemy-redshift uses the sqlalchemy test harness at this point in time, but after taking a quick look I agree--it seems like a much more scalable (and clean) approach going forward. This could be worth looking into more in the future.

Comment on lines 11 to 12
from sqlalchemy.dialects.postgresql.psycopg2 import PGDialect_psycopg2
from sqlalchemy.dialects.postgresql import (
psycopg2, psycopg2cffi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 291d88e

@@ -882,6 +866,49 @@ def _get_all_constraint_info(self, connection, **kw):
return all_constraints


class PsycopgRedshiftDialectMixin(RedshiftDialectMixin):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 96b9af0

@@ -882,6 +866,49 @@ def _get_all_constraint_info(self, connection, **kw):
return all_constraints


class PsycopgRedshiftDialectMixin(RedshiftDialectMixin):
"""
Define Psycopg specific behavior.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 17c6a6b

return cargs, default_args


class Psycopg2RedshiftDialect(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 29d5704

@jklukas
Copy link
Member

jklukas commented Sep 1, 2021

Thanks for all the updates, @Brooke-white. I have it on my to-do list to give this a full review, but probably won't happen until Friday.

@jklukas
Copy link
Member

jklukas commented Sep 7, 2021

Thanks for all the updates, @Brooke-white. I have it on my to-do list to give this a full review, but probably won't happen until Friday.

Even more things came up last week, so apologies that this will need to be further delayed.

@Brooke-white
Copy link
Contributor Author

Thanks for the update @jklukas , do you have an ETA for when you'll be able to give a full review?

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

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

Changes look good, including the new names and the shims for compatibility with RedshiftDialect. I've kicked off integration tests and will merge assuming those all come back clean.

@jklukas jklukas merged commit 7cbb98b into sqlalchemy-redshift:main Sep 9, 2021
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.

None yet

4 participants