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

SQLAlchemy v1.4.x support #221

Merged
merged 1 commit into from
Jul 6, 2021
Merged

Conversation

att14
Copy link
Contributor

@att14 att14 commented May 28, 2021

Fixes #220 and adds general support for SQLAlchemy v1.4.0. In my testing v1.2.x of SQLAlchemy no longer works so I dropped support for it.

Todos

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

@att14 att14 force-pushed the sa14 branch 5 times, most recently from 6122e18 to 6ccb56f Compare May 28, 2021 02:34
@att14 att14 mentioned this pull request May 28, 2021
4 tasks
@att14
Copy link
Contributor Author

att14 commented May 28, 2021

@jklukas should I pull in the changes from your sqa-1.4 branch?

@jklukas
Copy link
Member

jklukas commented May 31, 2021

Thanks so much for taking this on!

I triggered integration tests. You can see errors at: https://travis-ci.org/github/sqlalchemy-redshift/sqlalchemy-redshift/builds/773070665

@jklukas should I pull in the changes from your sqa-1.4 branch?

Sure, if those changes look reasonable. I'm happy to abandon that branch in favor of your efforts here. The dburl changes look like they will fix some of the errors on your fork at least.

if sa_version <= Version('1.4.0'):
del kw['identity']
else:
kw['identity'] = None
Copy link
Contributor Author

@att14 att14 Jun 2, 2021

Choose a reason for hiding this comment

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

@jklukas the function that finds these kwargs is deep in the Redshift/Postgres innards and I don't have familiarity with it right now, so I didn't support this argument. If you think that's a problem, can point me in the right direction?

Copy link
Member

Choose a reason for hiding this comment

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

What's the problem that this is addressing? Is existence of an "identity" kwarg causing failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, you are probably right that I don't need to do this, however, this is called from get_columns which eventually uses the following query to get the kwargs to this function.

            SELECT
              n.nspname as "schema",
              c.relname as "table_name",
              att.attname as "name",
              format_encoding(att.attencodingtype::integer) as "encode",
              format_type(att.atttypid, att.atttypmod) as "type",
              att.attisdistkey as "distkey",
              att.attsortkeyord as "sortkey",
              att.attnotnull as "notnull",
              pg_catalog.col_description(att.attrelid, att.attnum)
                as "comment",
              adsrc,
              attnum,
              pg_catalog.format_type(att.atttypid, att.atttypmod),
              pg_catalog.pg_get_expr(ad.adbin, ad.adrelid) AS DEFAULT,
              n.oid as "schema_oid",
              c.oid as "table_oid"
            FROM pg_catalog.pg_class c
            LEFT JOIN pg_catalog.pg_namespace n
              ON n.oid = c.relnamespace
            JOIN pg_catalog.pg_attribute att
              ON att.attrelid = c.oid
            LEFT JOIN pg_catalog.pg_attrdef ad
              ON (att.attrelid, att.attnum) = (ad.adrelid, ad.adnum)
            WHERE n.nspname !~ '^pg_'
              AND att.attnum > 0
              AND NOT att.attisdropped
            UNION
            SELECT
              view_schema as "schema",
              view_name as "table_name",
              col_name as "name",
              null as "encode",
              col_type as "type",
              null as "distkey",
              0 as "sortkey",
              null as "notnull",
              null as "comment",
              null as "adsrc",
              null as "attnum",
              col_type as "format_type",
              null as "default",
              null as "schema_oid",
              null as "table_oid"
            FROM pg_get_late_binding_view_cols() cols(
              view_schema name,
              view_name name,
              col_name name,
              col_type varchar,
              col_num int)
            ORDER BY "schema", "table_name", "attnum";

I don't know enough about these tables to know where to get the identity information from. I will remove this else, but any guidance on getting identity information in the query above would be useful. Or if you think I can leave this functionality out, then that's fine too.

Copy link
Member

Choose a reason for hiding this comment

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

Does 1.4 require that "identity" exist in kwargs? If so, it's probably fine to set this to None if no "identity" key already 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.

No, it's not required. I'll make the 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.

On a side note, I think this function needs to be reevaluated. SQLAlchemy's _get_column_info only takes positional arguments. So if this is called somewhere inside SQLAlchemy that is not overridden by this library, this function will most likely throw an error like:

In [1]: def foo(identity):
   ...:     pass
   ...:

In [2]: foo('1', identity=None)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-8bf4e478a39f> in <module>
----> 1 foo('1', identity=None)

TypeError: foo() got multiple values for argument 'identity'

tox.ini Outdated Show resolved Hide resolved
@att14
Copy link
Contributor Author

att14 commented Jun 7, 2021

@jklukas can you kick off the integration tests again?

@att14
Copy link
Contributor Author

att14 commented Jun 7, 2021

@jklukas I think the previous failures left the cluster in a bad state. The error is Schema "other_schema" already exists, which I got locally when I was testing this after a failed run. I needed to run DROP SCHEMA other_schema CASCADE; for the tests to run again.

@jklukas
Copy link
Member

jklukas commented Jun 8, 2021

@jklukas I think the previous failures left the cluster in a bad state. The error is Schema "other_schema" already exists, which I got locally when I was testing this after a failed run. I needed to run DROP SCHEMA other_schema CASCADE; for the tests to run again.

We spin up a Redshift cluster on demand for tests which exists for a few hours and then spins down. I kicked off tests again today which brought up a fresh cluster, but it's still showing this same error about "other_schema" existing.

In any case, each test run should be generating a separate randomly-named database within the Redshift cluster, so different test runs shouldn't interfere with each other, even if they fail to clean up properly. I think there must be some change in commit semantics along with the 1.4 upgrade that causes issues either with the text code or with the actual reflection code.

My best guess is that these lines behave differently in 1.4: https://github.com/sqlalchemy-redshift/sqlalchemy-redshift/blob/master/tests/rs_sqla_test_utils/models.py#L8-L9

@att14
Copy link
Contributor Author

att14 commented Jun 9, 2021

Unfortunately, these were failing with SQLAlchemy 1.3. I should have fixed everything by cleaning up after some tests and being more defensive when creating other_schema (before_create executes before every CREATE statement). I'm actually not sure how these tests passed previously because tests/test_reflection_views.py creates the same table twice without dropping it.

@jklukas can you kick off the integration tests again?

@jklukas
Copy link
Member

jklukas commented Jun 9, 2021

Reran integration tests, and it looks like we're down to just one or two test cases that are failing, differing by python version. They might be race conditions, as I don't know why python version would affect it.

Travis isn't expanding this to have cases for both SA 1.3 and SA 1.4, so looks like the tests here are actually all running SA 1.3. Any thoughts on why it's not expanding to handle the sa14 cases?

@att14
Copy link
Contributor Author

att14 commented Jun 30, 2021

Not really sure what to do here. The tests all pass for all versions on my local machine. Can you explain more how https://bigcrunch.herokuapp.com/session/ works?

@jklukas
Copy link
Member

jklukas commented Jun 30, 2021

Not really sure what to do here. The tests all pass for all versions on my local machine. Can you explain more how https://bigcrunch.herokuapp.com/session/ works?

The code lives at https://github.com/sqlalchemy-redshift/bigcrunch

Bigcrunch is basically just responsible for spinning up a cluster if tests haven't been run in a while, and then there's a scheduled task to spin the cluster down if it hasn't been touched for some number of hours.

looks like the tests here are actually all running SA 1.3. Any thoughts on why it's not expanding to handle the sa14 cases?

I don't do much advanced tox configuration, so it's not clear to me whether the expansion is working as intended. Any ideas of how to get this to run the sa14 cases?

@att14
Copy link
Contributor Author

att14 commented Jun 30, 2021

The sa14 cases are running. You need to scroll down below the sa13 failures.

Could you trigger a rerun of one of the versions manually, so it isn't running in parallel with the other versions? For example, https://travis-ci.org/github/sqlalchemy-redshift/sqlalchemy-redshift/jobs/774005946 click "Trigger Build" on this page.

I just want to verify that it has nothing to do with concurrency because I cannot reproduce locally.

@jklukas
Copy link
Member

jklukas commented Jun 30, 2021

Could you trigger a rerun of one of the versions manually, so it isn't running in parallel with the other versions? For example, https://travis-ci.org/github/sqlalchemy-redshift/sqlalchemy-redshift/jobs/774005946 click "Trigger Build" on this page.

I just tried to rerun, and unfortunately it looks like travis-ci.org has finally stopped running builds and entered read-only mode. We'll need to transition to travis-ci.com before we can run builds again. I'll take a look tomorrow and evaluate how much effort that would be.

@jklukas
Copy link
Member

jklukas commented Jul 6, 2021

I'm pursuing https://docs.travis-ci.com/user/migrate/open-source-repository-migration to get CI running again.

@jklukas
Copy link
Member

jklukas commented Jul 6, 2021

CI is now up and running again on travis-ci.com; see https://travis-ci.com/github/sqlalchemy-redshift/sqlalchemy-redshift/builds/231932929

3.8 finished successfully, but others failed. I'm retrying 3.9 on its own.

I would expect that these separate builds shouldn't interfere with one another, considering they should each be provisioning a database with a random name and then tearing it down when they're finished. So I'm inclined to believe that test cases within a build are interfering with one another.

@jklukas
Copy link
Member

jklukas commented Jul 6, 2021

I would expect that these separate builds shouldn't interfere with one another, considering they should each be provisioning a database with a random name and then tearing it down when they're finished. So I'm inclined to believe that test cases within a build are interfering with one another.

I'm rerunning these one by one, and they've been succeeding. So I stand corrected. It certainly does look like these builds are interfering with one another when run concurrently.

@att14 att14 force-pushed the sa14 branch 2 times, most recently from 8ce533e to cfec829 Compare July 6, 2021 17:08
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.

Integration tests are passing! I'm going to merge, and I'll plan to make a release tomorrow.

@jklukas jklukas merged commit 4ecb81d into sqlalchemy-redshift:master Jul 6, 2021
@att14 att14 deleted the sa14 branch July 6, 2021 20:27
@att14 att14 restored the sa14 branch July 6, 2021 20:28
@att14 att14 changed the title SQLAlchemy v1.4.0 support SQLAlchemy v1.4.x support Jul 6, 2021
@jklukas
Copy link
Member

jklukas commented Jul 7, 2021

This is now released as 0.8.4

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.

Error when delete()ing rows returned by filter_by
3 participants