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

Incremental model show reason for on_schema_change fail failures #5505

Merged
merged 5 commits into from
Aug 12, 2022
Merged

Incremental model show reason for on_schema_change fail failures #5505

merged 5 commits into from
Aug 12, 2022

Conversation

ilanbenb
Copy link
Contributor

@ilanbenb ilanbenb commented Jul 20, 2022

resolves issue #5504

Description

When the incremental model fails, I do not get the context I need to easily fix my discrepancy.
Adding more verbosity.

Checklist

When the incremental model fails, I do not get the context I need to easily fix my discrepency.
Adding more info
@ilanbenb ilanbenb requested a review from a team July 20, 2022 18:08
@ilanbenb ilanbenb requested a review from a team as a code owner July 20, 2022 18:08
@cla-bot
Copy link

cla-bot bot commented Jul 20, 2022

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @ilanbenb

@ilanbenb
Copy link
Contributor Author

Hello,
This is my first contribution to this repo.
This first commit is a preliminary proposition and should be a conversation opener.
Thank you,
Ilan

@ilanbenb
Copy link
Contributor Author

for

Done :)

@cla-bot cla-bot bot added the cla:yes label Jul 20, 2022
Added changie changes
@ilanbenb ilanbenb requested a review from a team as a code owner July 20, 2022 18:41
@jtcohen6 jtcohen6 added Team:Adapters Issues designated for the adapter area of the code ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Jul 21, 2022
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

This looks good to me! @ilanbenb Thanks for adding this!

@ChenyuLInx
Copy link
Contributor

I will leave this to the adapter team to take a second pass and merge(assuming all tests passes, I don't see any reason they won't)

Comment on lines 123 to 126
Extra info for easy anomaly detection:
Source columns not in target: {{ schema_changes_dict['source_not_in_target'] }}
Target columns not in source: {{ schema_changes_dict['target_not_in_source'] }}
New column types: {{ schema_changes_dict['new_target_types'] }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@dbeatty10 @dataders Does either of you have thoughts on the specific language here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like "Additional troubleshooting context:" could be a drop-in replacement for "Extra info for easy anomaly detection:"

Side note: it's a bit unfortunate that "schema" is both the 2nd level in a three-level database.schema.relation hierarchy as well as the result of DDL operations on a relation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbeatty10 I updated the log message per your request. It looks better

Log message text  enhancement
@ilanbenb ilanbenb changed the title show reason for schema change failures Incremental model show reason for on_schema_change fail failures Jul 29, 2022
@ilanbenb ilanbenb requested review from dbeatty10 and jtcohen6 July 30, 2022 10:30
@ilanbenb
Copy link
Contributor Author

ilanbenb commented Aug 7, 2022

Hey, @VersusFacit @jtcohen6 This pr is stall for more than 15 days.
Can it be reviewed by the code owners so it can be merged in the near future?

Thanks,
Ilan

@ChenyuLInx ChenyuLInx merged commit 7efb6ab into dbt-labs:main Aug 12, 2022
@ChenyuLInx
Copy link
Contributor

@ilanbenb sorry for the delay! I am merging this for the the Adapter team as all feedback has been addressed. Thanks for the contribution!

@ilanbenb ilanbenb deleted the patch-1 branch August 12, 2022 22:35
VersusFacit pushed a commit that referenced this pull request Sep 5, 2022
* show reason for schema change failures

When the incremental model fails, I do not get the context I need to easily fix my discrepency.
Adding more info

* Update on_schema_change.sql

Fix identation

* Added changie changes

Added changie changes

* Update on_schema_change.sql

Trim whitespaces

* Update on_schema_change.sql

Log message text  enhancement
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
…-labs#5505)

* show reason for schema change failures

When the incremental model fails, I do not get the context I need to easily fix my discrepency.
Adding more info

* Update on_schema_change.sql

Fix identation

* Added changie changes

Added changie changes

* Update on_schema_change.sql

Trim whitespaces

* Update on_schema_change.sql

Log message text  enhancement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants