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

Fix: Create table statement parameter ordering when configured with backup & dist/sort #63

Merged
merged 13 commits into from Mar 3, 2022

Conversation

SMeltser
Copy link
Contributor

@SMeltser SMeltser commented Jan 11, 2022

resolves #60

Description

This PR corrects the parameter ordering of the table creation statement to accommodate when a model is configured with both a BACKUP parameter as well as an accompanying DIST/SORT key.

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 Jan 11, 2022
@SMeltser SMeltser marked this pull request as ready for review January 11, 2022 07:44
@McKnight-42 McKnight-42 self-requested a review January 25, 2022 20:46
@McKnight-42
Copy link
Contributor

@SMeltser if you could update your local branch and repush up to test with most recent changes to main we should be able to get this approved.

@VersusFacit
Copy link
Contributor

👍 To what Matt said

Since this was let through before because we didn't have a test for this case, would you mind adding a test or two for queries which include backup and various other Redshift options?

Not to send you out into deep end by yourself. Here's the class or thereabouts a test has both the backup option and a dist key/sort key should go!

We'll be happy to lend extra support in making sure this behavior gets recorded as a regression test.

@SMeltser
Copy link
Contributor Author

Will do ASAP! Thank you for the feedback @McKnight-42 @VersusFacit

@SMeltser
Copy link
Contributor Author

@McKnight-42 @VersusFacit Just completed the test updates. Open to any and all feedback you have regarding the setup. I incorporated a conditional flow if the backup param is expected to ensure that the ordering of parameters is valid when dist/sortkey's are supplied.

Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

Hey, this is great! Thanks for taking a stab at this and following up. We're so close to finishing this up.

Just two minor comments. I really like the trick of checking things by index in the over DDL string. Quality way to ensure there's a proper ordering at play.

@will-peloton
Copy link

will-peloton commented Feb 17, 2022

thank you all for working on this fix. This may be a bit off topic but when can we expect 1.0.1 to be released? I'm really looking forward to using this.

@VersusFacit
Copy link
Contributor

@SMeltser We had a few bugs crop up over the last week or so that are responsible for unit tests failures in your logs. Thankfully, we've already fixed them.

Merging the adapter root branch into this project will resolve them and get you green, at which point I'll (finally) approve for.

@will-peloton As soon as we get this green, I think we can merge it. As for 1.0.1 of this adapter (if that's what you mean; please correct me if I'm misreading), we're very much in a 'keeping the lights on' mode right now. That said, I'll report back to the team a desire to see that initiative pushed through.

Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

Nicely done!!! So glad to see this completed :)) Thanks for sticking with it 🙇‍♀️ 🚀

edit: integration tests didn't run on this test and won't even with a manual rerun. To compensate, I locally setup and tested your branch. As expected, all green!
image

Merging :shipit:

@VersusFacit VersusFacit merged commit 4b81b0b into dbt-labs:main Mar 3, 2022
McKnight-42 pushed a commit that referenced this pull request Mar 25, 2022
@McKnight-42 McKnight-42 mentioned this pull request Mar 25, 2022
4 tasks
leahwicz pushed a commit that referenced this pull request Mar 31, 2022
* backport of pr #63

* Empty-Commit

* removing unneeded changelog entry

Co-authored-by: Steven Meltser <31104631+SMeltser@users.noreply.github.com>
abbywh pushed a commit to abbywh/dbt-redshift that referenced this pull request Oct 11, 2023
…ackup & dist/sort (dbt-labs#63)

* set BACKUP parameter before dist/sort keys
* updated changelog
* added test v1
* Finalize backup tests
* update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-23] syntax error with table backup option
4 participants