Navigation Menu

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

Add RedshiftDialect_redshift_connector #232

Merged

Conversation

Brooke-white
Copy link
Contributor

Adds a dialect to support use of redshift_connector database driver.

Modifies sqlalchemy-redshift init requirement for psycopg2 to dialect dbapi method to avoid raising an ImportError for users using RedshiftDialect_redshift_connector

Modifies pytest to accept a command line option, dbdriver, to provide 1 or more database drivers to run the test suite against (e.g. —dbdriver psycopg2 —dbdriver redshift_connector). By default tests will run for psycopg2 and psycopg2cffi

Modifies tox configuration to test with —dbdriver redshift_connector for compatible Python versions (i.e. py36 >=), and to use a version of requests.

Reworks test_dialects.py to use a mock sqlalchemy engine.

Todos

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

@Brooke-white Brooke-white marked this pull request as draft September 10, 2021 23:19
@Brooke-white
Copy link
Contributor Author

These changes to the tests seem to be slowing down things quite a bit, I'll take another look on Monday.

@@ -900,7 +901,16 @@ def create_connect_args(self, *args, **kwargs):
class RedshiftDialect_psycopg2(
Psycopg2RedshiftDialectMixin, PGDialect_psycopg2
):
pass
@classmethod
def dbapi(cls):
Copy link
Member

Choose a reason for hiding this comment

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

Can this be defined in Psycopg2RedshiftDialectMixin or does it have to be a classmethod on each concrete dialect? I assume this is some hook in the SQLAlchemy machinery, but I'm struggling to find documentation about a dbapi classmethod. Can you point to background on this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could move this to Psycopg2RedshiftDialectMixin if we did something like this, using the value of driver defined by the dialect (e.g. defined here for the psycopg2cfifi dialect).

class Psycopg2RedshiftDialectMixin(RedshiftDialectMixin): 
   @classmethod
    def dbapi(cls):
        try:
            return __import__(self.driver)
        except ImportError:
            raise ImportError(
                'No module named {}.'.format(self.driver)
            )

For the dbapi classmethod, looking to the DefaultDialect class (from which PGDialect inherits from) it is used in DefaultDialect.connect(), as well as for defining the dbapi paramstyle that will be used.

I was unable to find any docs for the dpapi classmethod either, as I stumbled upon it when looking for where the driver's connect method is called.

@Brooke-white Brooke-white marked this pull request as ready for review September 15, 2021 16:49
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.

This looks good. I've kicked off integration tests. Do you have any more info on duration of tests with this change?

@classmethod
def dbapi(cls):
try:
return __import__(cls.driver)
Copy link
Member

Choose a reason for hiding this comment

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

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 edfb456

@Brooke-white
Copy link
Contributor Author

Do you have any more info on duration of tests with this change?

It seems like the tests which make a connection to a Redshift cluster take about twice as long. The unit tests are pretty quick. I think this has something to do with the dynamic parameterization of the fixtures, as the slow down came once I introduced that logic.

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.

LGTM. Kicked off integration tests again and will merge when done.

@jklukas jklukas merged commit d43b544 into sqlalchemy-redshift:main Sep 20, 2021
@Brooke-white
Copy link
Contributor Author

thanks for the quick turn around, @jklukas! When do you plan on making the next release? We'd like to get this feature out to customers as soon as we can.

@jklukas
Copy link
Member

jklukas commented Sep 22, 2021

I just pushed 0.8.6 which includes this new connector.

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

2 participants