-
Notifications
You must be signed in to change notification settings - Fork 60
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 Redshift parameter to create tables with backup option specified #42
Conversation
@jtcohen6 I was hoping the tests would run in CI to prove this out a bit, but I'm a first-time contributor. I don't have a Redshift database to do much with local testing. Let me know your thoughts in general and if there's anything that needs to be added. |
Closing and reopening to trigger adapter integration tests |
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.
Thanks for reopening the PR here @dlb8685!
Looks like the tests are failing in a few small ways, should be quick to fix up. We're cutting a second (and last) RC of v1.0 very soon, so there's a good chance this goes into v1.1 instead, which is totally ok :)
|
||
@use_profile('redshift') | ||
def test__redshift_backup_table_option(self): | ||
self.assertEqual(len(self.run_dbt(["seed"])), 1) |
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.
There aren't any seeds defined in the project
self.assertEqual(len(self.run_dbt(["seed"])), 1) |
def test__redshift_backup_table_option_project_config_false(self): | ||
# Update project config to set backup to False. | ||
# This should make the 'model_backup_undefined' switch to BACKUP NO | ||
self.project_config_append('models', {'backup': False}) |
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.
Looks like this is causing an error in the integration test:
def project_config_append(self, new_name, new_element):
> self._project_config[new_name] = new_element
E AttributeError: 'TestBackupTableOption' object has no attribute '_project_config'
Much simpler would be to create a separate test case, and define the config anew:
TestBackupTableOptionProjectFalse(TestBackupTableOption):
@property
def project_config(self):
return {
'config-version': 2,
'models': {'backup': False}
}
@use_profile('redshift')
def test__redshift_backup_table_option_project_config_false(self):
...
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.
I made this change and also removed the lines that were running the seed. Let me know if this looks good and if the tests can run again.
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.
@dlb8685 Nice work! I think we might be able to sneak this in for v1.0.0rc2 after all :)
Just added you as a contributor in the changelog, will merge as soon as the tests pass.
Thank you! @jtcohen6 |
* Slack message for failed nightly runs (#41) * Add Redshift parameter to create tables with backup option specified (#42) * Update impl and adapters to support backup parameter * Add test files * Add test files * Add PR link to Changelog * Add EOF newlines * Debug and split test into two separate cases * Add contributor info Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * Bumping version to 1.0.0rc2 (#45) * Bumping version to 1.0.0rc2 * Update changelog Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Dan Bryan <dlb8685@gmail.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com>
I reported a bug I think that is related to this #60. I believe the fix is moving backup option up 2 lines in the adapter.sql before setting dist_key. |
#80) * Merge `main` into `1.0.latest` (#46) * Slack message for failed nightly runs (#41) * Add Redshift parameter to create tables with backup option specified (#42) * Update impl and adapters to support backup parameter * Add test files * Add test files * Add PR link to Changelog * Add EOF newlines * Debug and split test into two separate cases * Add contributor info Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * Bumping version to 1.0.0rc2 (#45) * Bumping version to 1.0.0rc2 * Update changelog Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Dan Bryan <dlb8685@gmail.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> * [Backport] Bumping version to 1.0.0 (#47) (#48) * Bumping version to 1.0.0 (#47) Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> * Update CHANGELOG.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> * Fix package version (#49) * using string interpoloation to gather correct pointer for dbt-core tests against release branches * created new job for gha to grab correct version of dbt-core to test branch against * minor update * adding Get dbt-core-version step to integration.yaml * modifying version parameters * change for integration testing * updating file * readding pull_request_target now that tests pass * make nit: suggested changes * testing conditional logic in integration.yml * updating test names * creating main.yml versions of new condtional steps for dbt-version gather * trying different version of test v.2 * v.3 of conditional mix of original version of tests and leah logic * adding comment and changelog entry * changes made after review by @VersusFacit and @kwigley * name change * minor updates * updating name of version ref * name change of dbt-version step to dbt-core-ref to be more descriptive iof where version is coming from * Update test_backup_table_option.py * Update test_backup_table_option.py * reseting file that shouldn't of been changed Co-authored-by: leahwicz <60146280+leahwicz@users.noreply.github.com> Co-authored-by: Dan Bryan <dlb8685@gmail.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com>
…dbt-labs#42) * Update impl and adapters to support backup parameter * Add test files * Add test files * Add PR link to Changelog * Add EOF newlines * Debug and split test into two separate cases * Add contributor info Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
dbt-labs#80) * Merge `main` into `1.0.latest` (dbt-labs#46) * Slack message for failed nightly runs (dbt-labs#41) * Add Redshift parameter to create tables with backup option specified (dbt-labs#42) * Update impl and adapters to support backup parameter * Add test files * Add test files * Add PR link to Changelog * Add EOF newlines * Debug and split test into two separate cases * Add contributor info Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * Bumping version to 1.0.0rc2 (dbt-labs#45) * Bumping version to 1.0.0rc2 * Update changelog Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Dan Bryan <dlb8685@gmail.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> * [Backport] Bumping version to 1.0.0 (dbt-labs#47) (dbt-labs#48) * Bumping version to 1.0.0 (dbt-labs#47) Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> * Update CHANGELOG.md Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> * Fix package version (dbt-labs#49) * using string interpoloation to gather correct pointer for dbt-core tests against release branches * created new job for gha to grab correct version of dbt-core to test branch against * minor update * adding Get dbt-core-version step to integration.yaml * modifying version parameters * change for integration testing * updating file * readding pull_request_target now that tests pass * make nit: suggested changes * testing conditional logic in integration.yml * updating test names * creating main.yml versions of new condtional steps for dbt-version gather * trying different version of test v.2 * v.3 of conditional mix of original version of tests and leah logic * adding comment and changelog entry * changes made after review by @VersusFacit and @kwigley * name change * minor updates * updating name of version ref * name change of dbt-version step to dbt-core-ref to be more descriptive iof where version is coming from * Update test_backup_table_option.py * Update test_backup_table_option.py * reseting file that shouldn't of been changed Co-authored-by: leahwicz <60146280+leahwicz@users.noreply.github.com> Co-authored-by: Dan Bryan <dlb8685@gmail.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com>
resolves #18
Description
This PR adds a config parameter "backup" that controls whether a Redshift table will be created with
BACKUP No
or not. Current default (both in DBT and more generally with Redshift) is for this BACKUP to be set to True unless explicitly overridden. But, for some transient and intermediate tables, there can be no need for backup and users would like to be able to set config accordingly.The config parameter is added in
impl.py
and the statements inadapters.sql
are updated to use this parameter.Checklist
I don't have a Redshift instance to run this code on. I will investigate if the unit tests fail, but based on nearly identical old PR and local linting I'm optimistic.
CHANGELOG.md
and added information about my change to the "dbt-redshift next" section.