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

Update redshift-configs.md #3822

Merged
merged 19 commits into from
Aug 7, 2024
Merged

Update redshift-configs.md #3822

merged 19 commits into from
Aug 7, 2024

Conversation

Rafalem29
Copy link
Contributor

@Rafalem29 Rafalem29 commented Jul 26, 2023 •
edited
Loading

The existing sort key and dist key examples don't really align with Redshift's best practices. I wanted to propose changes that imply low-cardinality sort keys (which are preferable) and very high cardinality dist keys to ensure parallelism.

What are you changing in this pull request and why?

Checklist

  • Review the Content style guide and About versioning so my content adheres to these guidelines.
  • Add a checklist item for anything that needs to happen before this PR is merged, such as "needs technical review" or "change base branch."

Adding new pages (delete if not applicable):

  • Add page to website/sidebars.js
  • Provide a unique filename for the new page

Removing or renaming existing pages (delete if not applicable):

  • Remove page from website/sidebars.js
  • Add an entry website/static/_redirects
  • Ran link testing to update the links that point to the deleted page

@Rafalem29
Update redshift-configs.md
af5c300 
The existing sort key and dist key examples don't really align with Redshift's best practices. I wanted to propose changes that imply low-cardinality sort keys (which are preferable) and very high cardinality dist keys to ensure parallelism.
@Rafalem29 Rafalem29 requested review from a team and dataders as code owners July 26, 2023 15:50
@welcome
Copy link

welcome bot commented Jul 26, 2023

Hello!👋 Thanks for contributing to the dbt product documentation and opening this pull request! ✨
We use Markdown and some HTML to write the dbt product documentation. When writing content, you can use our style guide and content types to understand our writing standards and how we organize information in the dbt product docs.
We'll review your contribution and respond as soon as we can. 😄

@netlify
Copy link

netlify bot commented Jul 26, 2023 •
edited
Loading

✅ Deploy Preview for docs-getdbt-com ready!

Name Link
🔨 Latest commit 280af09
🔍 Latest deploy log https://app.netlify.com/sites/docs-getdbt-com/deploys/65032857612165000861ce01
😎 Deploy Preview https://deploy-preview-3822--docs-getdbt-com.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added content Improvements or additions to content new contributor Label for first-time contributors size: small This change will take 1 to 2 days to address labels Jul 26, 2023
@vercel
Copy link

vercel bot commented Sep 13, 2023

@dataders is attempting to deploy a commit to the dbt-labs Team on Vercel.

A member of the Team first needs to authorize it.

dataders
dataders requested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@dataders dataders left a comment

Choose a reason for hiding this comment

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

changes that imply low-cardinality sort keys (which are preferable) and very high cardinality dist keys to ensure parallelism.

I'm all for this change, but can we go one step further beyond "imply"? Can we not add a sentence that explicitly calls out this best practice?

@Rafalem29
Copy link
Contributor Author

Rafalem29 commented Sep 13, 2023 •
edited
Loading

Oh certainly, I'm in favor of calling this out in the most plain-spoken, clear way possible. I just didn't want to duplicate effort or re-invent (sorry, accidental pun) the AWS Redshift documentation.

@dataders
Copy link
Contributor

Oh certainly, I'm in favor of calling this out in the most plain-spoken, clear way possible. I just didn't want to duplicate effort or re-invent (sorry, accidental pun) the AWS Redshift documentation.

I'll take any language you propose!

@Rafalem29
Copy link
Contributor Author

I think the links to AWS documentation and the modifications to the examples to point to what are, presumably, low-cardinality columns should suffice for now.

@vercel
Copy link

vercel bot commented Sep 20, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 7, 2024 3:57pm

@matthewshaver
Update redshift-configs.md
fc3ec85 
Adding link to Redshift best practices
@matthewshaver
Copy link
Contributor

Thank you @Rafalem29 Sorry this took so long to implement

@matthewshaver matthewshaver requested a review from dataders August 6, 2024 18:11
@matthewshaver matthewshaver disabled auto-merge August 6, 2024 21:39
@matthewshaver matthewshaver merged commit eb176c2 into dbt-labs:current Aug 7, 2024
3 checks passed
@matthewshaver
Copy link
Contributor

Author had addressed previous comments. Went ahead with the merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewshaver matthewshaver matthewshaver approved these changes

@dataders dataders Awaiting requested review from dataders

Assignees
No one assigned
Labels
content Improvements or additions to content new contributor Label for first-time contributors size: small This change will take 1 to 2 days to address
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Rafalem29 @dataders @matthewshaver @mirnawong1 @runleonarun