Fix/incremental column precision changes #5395
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
| @@ -63,6 +63,12 @@ def run_incremental_sync_remove_only(self): | |||
| compare_target = 'incremental_sync_remove_only_target' | |||
| self.run_twice_and_assert(select, compare_source, compare_target) | |||
|
|
|||
| def run_incremental_sync_shorter_cols(self): | |||
| select = 'model_a incremental_sync_shorter_columns incremental_sync_shorter_target' | |||
There was a problem hiding this comment.
@epapineau thanks for contributing and adding the tests! I tried to run the test locally and it actually didn't pass. incremental_sync_shorter_columns_target should be the last model selected here I think. But even after adjusting it still fails. Do you think you can update the test case a bit so that it replicates the issue that happened in #5395 and would fail without your change in the macro but passes afterwards?
There was a problem hiding this comment.
BTW, this function won't actually run unless you have a test function that actually runs it. something like
@use_profile('postgres')
def test__postgres__run_incremental_sync_shorter_cols(self):
self.run_incremental_sync_shorter_cols()There was a problem hiding this comment.
Thanks for the feedback @ChenyuLInx. I don't think this bug exists on postgres - I was only able to recreate on Snowflake & Redshift (didn't try on BigQuery). Am I able to pass a different test db as a value for @use_profile()?
There was a problem hiding this comment.
Gotcha, we don't really have another option in dbt-core, so I think we should probably just add the code here, and add a test in dbt-snowflake or dbt-redshift separately.
There was a problem hiding this comment.
@ChenyuLInx Thanks for your feedback on Friday. I took a stab at writing the test in dbt/tests/adapter, let me know what you think 🎉
There was a problem hiding this comment.
@epapineau Thanks for moving the test into an adapter test!!
This all looks good! Just want to double-check, running the test in dbt-snowflake/redshift before your fix would cause the test to fail right?
| class Testincremental(BaseIncremental): | ||
| pass | ||
|
|
||
|
|
||
| class TestBaseIncrementalNotSchemaChange(BaseIncrementalNotSchemaChange): |
There was a problem hiding this comment.
Great!! Can we also add this test in dbt-snowflake and dbt-redshift?
jtcohen6
added
ready_for_review
|
@epapineau is this PR ready to merge in? We are getting ready to cut the RC branch for 1.2 soon so just trying to get any last PRs in before that happens |
|
@leahwicz Sorry for the delay in responding, I'm just getting caught back up after two weeks of PTO. Let me confirm the test fails as @ChenyuLInx asked and then this is ready to merge |
|
Paired with @epapineau today and adjusted the test to make sure it will fail on snowflake before applying the fix |
* Only consider schema change when column cannot be expanded * Add test for column shortening * Add changelog entry * Move test from integration to adapter tests * Remove print statement * add on_schema_change
* Only consider schema change when column cannot be expanded * Add test for column shortening * Add changelog entry * Move test from integration to adapter tests * Remove print statement * add on_schema_change
resolves #5351
Description
Consider whether column may be successfully expanded when qualifying schema changes
Checklist
changie newto create a changelog entry