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

Bug fixes for views: return views in get_table_options and improve reflection #46

Merged
merged 1 commit into from Sep 16, 2015

Conversation

@jklukas
Copy link
Collaborator

@jklukas jklukas commented Sep 9, 2015

Because SQLAlchemy only has limited support for views, some methods that include table in the name are also supposed to return views.

@jklukas jklukas force-pushed the view-fixes branch Sep 9, 2015
@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 9, 2015

Since there's little distinction between tables and views, this PR also includes a refactor to reduce duplication of code between table-specific and view-specific methods.

@jklukas jklukas force-pushed the view-fixes branch Sep 10, 2015
@jklukas jklukas changed the title Bug fix: return views in get_table_options Bug fixes for views: return views in get_table_options and improve reflection Sep 10, 2015
@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 11, 2015

@@ -643,7 +623,8 @@ def _get_all_table_and_view_info(self, connection, **kw):
AS "diststyle",
c.relowner AS "owner_id",
u.usename AS "owner_name",
pg_get_viewdef(c.oid) AS "view_definition",
TRIM(TRAILING ';' FROM pg_catalog.pg_get_viewdef(c.oid, true))

This comment has been minimized.

@graingert

graingert Sep 11, 2015
Collaborator

What's this trim for?

This comment has been minimized.

@jklukas

jklukas Sep 11, 2015
Author Collaborator

The output of pg_get_viewdef includes a semicolon at the end of the query. The SQLAlchemy CreateTable, etc. commands don't include a semicolon at the end of the output, so we need to strip that out somewhere for consistency.

This TRIM(TRAILING ';' is the solution used in psql's \d+ command to get rid of the semicolon.

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 11, 2015

This doesn't change functionality at all right?

@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 11, 2015

This doesn't change functionality at all right?

With this change, get_view_definition now doesn't include a semicolon in its output and get_table_options now returns an empty dict when the input relation is a view (previously, this raised an exception).

Both of the previous behaviors I consider bugs.

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 11, 2015

Can you add tests and changelog for these bugs please?

@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 11, 2015

I'm going to need some help if we're going to try to add tests for these. The current test structure for reflection relies on declarative_base objects, which AFAIK can't create views.

@graingert - Do you have suggestions about how we can create a view in our test models to reflect from?

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 11, 2015

One option is to use the redshift_engine pytest fixture and create views
on that using raw SQL. Beware this fixture is slower than
redshift_session because it generates a new database for each test
function.
On 11 Sep 2015 21:51, "Jeff Klukas" notifications@github.com wrote:

I'm going to need some help if we're going to try to add tests for these.
The current test structure for reflection relies on declarative_base
objects, which AFAIK can't create views.

@graingert https://github.com/graingert - Do you have suggestions about
how we can create a view in our test models to reflect from?


Reply to this email directly or view it on GitHub
#46 (comment)
.

@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 14, 2015

Test failure is an unexpected 500 from bigcrunch again.

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 14, 2015

For now you'll have to retry the build after redshift has booted up properly
On 14 Sep 2015 16:44, "Jeff Klukas" notifications@github.com wrote:

Test failure is an unexpected 500 from bigcrunch again.


Reply to this email directly or view it on GitHub
#46 (comment)
.

@jklukas jklukas force-pushed the view-fixes branch Sep 14, 2015
.compile(engine))


def test_view_reflection(redshift_engine):

This comment has been minimized.

@graingert

graingert Sep 15, 2015
Collaborator

nice

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 15, 2015

@bouk @cpcloud @jklukas I'm pro merging this, once it has a changelog entry

@jklukas jklukas force-pushed the view-fixes branch 2 times, most recently Sep 15, 2015
@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 15, 2015

Totally forgot about the changelog. I just pushed an update that adds a note about the changes.

@jklukas jklukas force-pushed the view-fixes branch Sep 15, 2015
@graingert
graingert reviewed Sep 15, 2015
View changes
CHANGES.rst Outdated
@@ -29,6 +30,7 @@
- Raise ``NoSuchTableError`` when trying to reflect a table that doesn't exist.
(`Issue #38 <https://github.com/graingert/redshift_sqlalchemy/issues/38>`_)


This comment has been minimized.

@graingert

graingert Sep 15, 2015
Collaborator

this file is auto-generated by zest.releaser do we need this extra new-line?

This comment has been minimized.

@jklukas

jklukas Sep 15, 2015
Author Collaborator

The newline as an attempt to make the whitespacing consistent. Other sections in this file are preceeded by two blank lines. It looks like I missed adding an extra blank line beneath the 0.2.1 section while I was at it.

It doesn't matter functionally, so I'll revert the change to avoid noise in the diff.

@jklukas jklukas force-pushed the view-fixes branch 2 times, most recently to ad64806 Sep 15, 2015
@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 16, 2015

We're still getting intermittent 500 responses from bigcrunch. How well do you understand the behavior, @graingert?

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 16, 2015

@jklukas there's a small window between the cluster booting and cluster being available (DNS etc) where these 500s occur. I know how to fix this. Are you able to access the rebuild button on Travis?

@jklukas jklukas force-pushed the view-fixes branch to ad64806 Sep 16, 2015
@jklukas
Copy link
Collaborator Author

@jklukas jklukas commented Sep 16, 2015

Are you able to access the rebuild button on Travis?

I have limited experience with Travis, so didn't know to look for one. I was just able to trigger a rebuild. We'll see how it does.

@graingert
Copy link
Collaborator

@graingert graingert commented Sep 16, 2015

This looks great, merging. Thanks very much!

graingert added a commit that referenced this pull request Sep 16, 2015
Bug fixes for views: return views in get_table_options and improve reflection
@graingert graingert merged commit c746d20 into master Sep 16, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@graingert graingert deleted the view-fixes branch Sep 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.