-
Notifications
You must be signed in to change notification settings - Fork 331
[view] Support CREATE OR REPLACE #710
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
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.
@mgorven Thanks for submitting this!
Would you add some tests that exercise this feature? It looks like these tests will be worthwhile:
ValueErroris raised whenmaterializedandreplaceare both true- A view is created when
replaceis true and it doesn't exist - A view is replaced when
replaceis true and it already exists
|
Sure. Are there any docs on how to run tests locally? It seems like they require a real database. |
|
That's definitely an area that could be improved. For now, I've kicked off CI for this PR. You should be able to commit additional changes and allow CI to run here. It appears that CI has started failing, so for the purposes of this PR please disregard the existing failures while working on new tests. Thanks! |
6816617 to
c852176
Compare
b7c1db9 to
1d6e0d7
Compare
|
As was discovered in #621, the tests don't actually create views (they create a table). I've changed the tests to run |
f10afbe to
c6fd76e
Compare
|
Only test failures are |
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.
One last thing that I finally realized: the replace parameter needs a bit of documentation in the create_view() docstring.
It'll be rendered automatically the next time the docs are built, and will appear like the other parameters in the create_view() documentation.
|
Thanks for your work on this, @mgorven! |
No description provided.