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.

Sign up for GitHub

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

Jump to bottom

Fix/bigquery job label length #3703

Merged
merged 17 commits into from
Aug 17, 2021
Merged

Fix/bigquery job label length #3703

merged 17 commits into from
Aug 17, 2021

Conversation

sungchun12
Copy link
Contributor

@sungchun12 sungchun12 commented Aug 6, 2021 •
edited
Loading

resolves #3612

Description

image
image

Screenshots based on using jaffle_shop dbt project along with using an example query comment macro

  • Updated the existing _sanitize_label function to validate a global length variable: _VALIDATE_LABEL_LENGTH_LIMIT
  • Added a passing test with 3 scenarios with randomly generated strings each time you run the test
  • If an exception is raised, it provides clearer logging in the terminal for the error reason being the length limit being exceeded along with the currently sanitized label
  • I will update the CHANGELOG.md after the core logic is approved

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@sungchun12 sungchun12 temporarily deployed to Postgres August 6, 2021 14:54 Inactive
@cla-bot
Copy link

cla-bot bot commented Aug 6, 2021

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: @sungchun12

@sungchun12 sungchun12 temporarily deployed to Snowflake August 6, 2021 14:54 Inactive
@sungchun12 sungchun12 temporarily deployed to Snowflake August 6, 2021 14:54 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 6, 2021 14:54 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 6, 2021 14:54 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 6, 2021 14:54 Inactive
@sungchun12 sungchun12 self-assigned this Aug 6, 2021
@cla-bot cla-bot bot added the cla:yes label Aug 6, 2021
@sungchun12 sungchun12 temporarily deployed to Postgres August 6, 2021 15:24 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 6, 2021 15:24 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 6, 2021 15:25 Inactive
@sungchun12 sungchun12 temporarily deployed to Snowflake August 6, 2021 15:25 Inactive
@sungchun12 sungchun12 temporarily deployed to Snowflake August 6, 2021 15:25 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 6, 2021 15:25 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 6, 2021 15:25 Inactive
@sungchun12 sungchun12 marked this pull request as ready for review August 6, 2021 15:37
@sungchun12
Copy link
Contributor Author

@nathaniel-may feel free to switch out reviewers. I added you since you were recommended and reviewed a similar pull request in the past: #3145

@nathaniel-may nathaniel-may requested a review from jtcohen6 August 6, 2021 19:59
@sungchun12 sungchun12 temporarily deployed to Snowflake August 16, 2021 13:56 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 16, 2021 13:56 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 16, 2021 13:56 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 16, 2021 13:56 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 16, 2021 13:56 Inactive
@sungchun12 sungchun12 temporarily deployed to Postgres August 16, 2021 14:18 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 16, 2021 14:18 Inactive
@sungchun12 sungchun12 temporarily deployed to Bigquery August 16, 2021 14:18 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 16, 2021 14:18 Inactive
@sungchun12 sungchun12 temporarily deployed to Redshift August 16, 2021 14:18 Inactive
@sungchun12 sungchun12 temporarily deployed to Snowflake August 16, 2021 14:18 Inactive
@sungchun12 sungchun12 temporarily deployed to Snowflake August 16, 2021 14:19 Inactive
@sungchun12
Copy link
Contributor Author

sungchun12 commented Aug 16, 2021 •
edited
Loading

@jtcohen6 I made all the updates you suggested and error handling is looking better than ever!

All the linting and BigQuery tests pass. I noticed the Redshift integration test suite failed because it couldn't install dependencies: here. Let me know if I need to resolve this error!

image

@sungchun12 sungchun12 requested a review from jtcohen6 August 16, 2021 17:12
jtcohen6
jtcohen6 approved these changes Aug 17, 2021
View reviewed changes
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @sungchun12 :)

I have no idea why that Redshift integration test failed on the first go-round, but I just re-ran it with success.

I forgot to ask you to add yourself to the list of contributors, which is very much deserved. Let's make sure to fix that in the final changelog before release.

@jtcohen6 jtcohen6 merged commit 2980cd1 into develop Aug 17, 2021
@jtcohen6 jtcohen6 deleted the fix/bigquery-job-label-length branch August 17, 2021 18:54
jtcohen6 added a commit that referenced this pull request Aug 17, 2021
@sungchun12 @jtcohen6
Fix/bigquery job label length (#3703)
c791777 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>
jtcohen6 added a commit that referenced this pull request Aug 17, 2021
@sungchun12 @jtcohen6
Fix/bigquery job label length (#3703)
454411e 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>
leahwicz pushed a commit that referenced this pull request Aug 18, 2021
@jtcohen6 @sungchun12
Fix/bigquery job label length (#3703) (#3766)
6fa30d1 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>

Co-authored-by: sungchun12 <[email protected]>
leahwicz pushed a commit that referenced this pull request Aug 18, 2021
@jtcohen6 @sungchun12
[Backport] #3703 to v0.20 (#3765)
3dc574e 
* Fix/bigquery job label length (#3703)

* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>

* Update changelog

Co-authored-by: sungchun12 <[email protected]>
leahwicz pushed a commit that referenced this pull request Aug 19, 2021
@jtcohen6 @sungchun12 @leahwicz
Fix/bigquery job label length (#3703) (#3766)
efc8ece 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>

Co-authored-by: sungchun12 <[email protected]>
IS-Josh pushed a commit to IS-Josh/dbt that referenced this pull request Sep 4, 2021
@sungchun12 @jtcohen6 @IS-Josh
Fix/bigquery job label length (dbt-labs#3703)
d44dcb2 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>
TeddyCr pushed a commit to TeddyCr/dbt that referenced this pull request Sep 9, 2021
@sungchun12 @jtcohen6 @TeddyCr
Fix/bigquery job label length (dbt-labs#3703)
a2bb285 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>
kwigley pushed a commit that referenced this pull request Sep 17, 2021
@jtcohen6 @sungchun12
Fix/bigquery job label length (#3703) (#3766)
dd75ee0 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>

Co-authored-by: sungchun12 <[email protected]>
kwigley pushed a commit that referenced this pull request Sep 17, 2021
@jtcohen6 @sungchun12
Fix/bigquery job label length (#3703) (#3766)
dcb3304 
* add blueprints to resolve issue

* revert to previous version

* intentionally failing test

* add imports

* add validation in existing function

* add passing test for length validation

* add current sanitized label

* remove duplicate var

* Make logging output 2 lines

Co-authored-by: Jeremy Cohen <[email protected]>

* Raise RuntimeException to better handle error

Co-authored-by: Jeremy Cohen <[email protected]>

* update test

* fix flake8 errors

* update changelog

Co-authored-by: Jeremy Cohen <[email protected]>

Co-authored-by: sungchun12 <[email protected]>
@leahwicz leahwicz mentioned this pull request Jun 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtcohen6 jtcohen6 jtcohen6 approved these changes

Assignees

@sungchun12 sungchun12

Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Raise a more helpful error if BigQuery job label is too long
2 participants
@sungchun12 @jtcohen6