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

Avoid truncating Redshift model names with more than 63 characters #147

Closed
wants to merge 5 commits into from

Conversation

Goodkat
Copy link
Contributor

@Goodkat Goodkat commented Aug 3, 2022

resolves #5586

Description

Because the dbt-redshift adapter inherits from dbt-postgres, it will use any new dbt-postgres methods/macros unless told otherwise. However Redshift allows relation names up to 127 characters (docs), so it doesn't require the same complex handling at the 63-character mark as it is done for Postgres.

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-redshift next" section.

@cla-bot cla-bot bot added the cla:yes label Aug 3, 2022
@jtcohen6 jtcohen6 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 3, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 3, 2022

@Goodkat Responding to your comment in the issue:

I am not sure about the tests, since I have no access to Redshift just now. Can we mock it?

We have access to a Redshift cluster running in CI. I think it would be simplest to add a functional test with the reproduction case from the original issue. In that example, the two models were named:

  • my_example__shipping_group_configurations__path_groups_and_settings__path_group__shipping_path_codes
  • my_example__shipping_group_configurations__path_groups_and_settings

@Goodkat
Copy link
Contributor Author

Goodkat commented Aug 3, 2022

@jtcohen6 I found out the existing tests for Redshift model names "my_name_is_XX_characters...":
https://github.com/dbt-labs/dbt-redshift/tree/main/tests/integration/relation_name_tests/models

We can extend them with the new entries for 127 characters or add there the cases from the original issue. What do you think?

@Goodkat
Copy link
Contributor Author

Goodkat commented Aug 3, 2022

And who maintains the CHANGELOG.md for this repository? It doesn't support changie here, right?
How to update it properly?

@troyharvey
Copy link

@Goodkat Example of how to update the CHANGELOG here: 9a3492a#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed

@VersusFacit
Copy link
Contributor

VersusFacit commented Aug 3, 2022

Thanks for taking a crack at this @Goodkat. Do you feel confident writing a test for this? I see you've got a good looking model for testing this but you'll need a Python driver. I also appreciate this is time sensitive. Happy to drive this over the finish line too if need be. 🏎️

@Goodkat
Copy link
Contributor Author

Goodkat commented Aug 4, 2022

Thanks @VersusFacit for your comments. I just found and extended the existing test models with the new case for 127 characters. I am not sure if it is enough for testing this.

@VersusFacit VersusFacit mentioned this pull request Aug 5, 2022
4 tasks
@VersusFacit
Copy link
Contributor

Hitting you back. I wanted to add a negative test to make sure the exception handling worked -- it does -- and in my PR, I also added tests in our new framework.

I'm closing this one out, but because I cherry picked your commits, your work will be rightfully highlighted in the history upon merge 💥

@VersusFacit VersusFacit closed this Aug 5, 2022
@Goodkat Goodkat deleted the issue/5586 branch August 6, 2022 10:09
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-964] [Regression] Truncate relation names when appending a suffix is creating cache inconsistency
4 participants