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

Redshift unit test limitation #5508

Merged
merged 9 commits into from
May 15, 2024
Merged

Redshift unit test limitation #5508

merged 9 commits into from
May 15, 2024

Conversation

matthewshaver
Copy link
Contributor

What are you changing in this pull request and why?

Closes: #5430

Checklist

Adding or removing pages (delete if not applicable):

  • Add/remove page in website/sidebars.js
  • Provide a unique filename for new pages
  • Add an entry for deleted pages in website/vercel.json
  • Run link testing locally with npm run build to update the links that point to deleted pages

@matthewshaver matthewshaver requested review from dataders and a team as code owners May 14, 2024 18:08
Copy link

vercel bot commented May 14, 2024

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 May 15, 2024 6:48pm

@github-actions github-actions bot added content Improvements or additions to content size: medium This change will take up to a week to address Docs team Authored by the Docs team @dbt Labs labels May 14, 2024
Copy link
Collaborator

@runleonarun runleonarun left a comment

Choose a reason for hiding this comment

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

@matthewshaver Just have some suggestion to smooth this message out.

website/docs/docs/build/unit-tests.md Show resolved Hide resolved
);

```
This results in the error:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This results in the error:
The previous code results in the error:

Copy link
Contributor Author

@matthewshaver matthewshaver May 14, 2024

Choose a reason for hiding this comment

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

Could going from "The following query" to the code example, to "the previous code" be a little redundant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@matthewshaver how about "This query results in the error:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me!

matthewshaver and others added 2 commits May 14, 2024 18:28
Co-authored-by: Leona B. Campbell <3880403+runleonarun@users.noreply.github.com>
Co-authored-by: Leona B. Campbell <3880403+runleonarun@users.noreply.github.com>

## Unit test limitations

Redshift doesn't support Unit tests when the SQL in the common table expression (CTE) contains functions such as `LISTAGG`, `MEDIAN`, `PERCENTILE_CONT`, etc. These functions must be executed against a user-created table. dbt combines given rows to be part of the CTE, which Redshift does not support.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@matthewshaver What I like about how the content is framed below is that it's clearer than this paragraph and it's framed in the positive. I think we should rewrite this paragraph so we:

  1. Communicate that unit test are limited by actually saying what they need to do to get unblocked. like "Avoid using SQL in CTE functions such as.." and "Unit tests can be used when you.."
  2. Explain how to overcome this limitation. Instead of the passive language of "These functions must be executed..." Use more active language like "Execute these functions against..."

Copy link
Collaborator

@runleonarun runleonarun left a comment

Choose a reason for hiding this comment

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

This works

@matthewshaver matthewshaver merged commit b390a97 into current May 15, 2024
11 checks passed
@matthewshaver matthewshaver deleted the redshift-unittest branch May 15, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Improvements or additions to content Docs team Authored by the Docs team @dbt Labs size: medium This change will take up to a week to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] Compute-node only functions in Redshift like MEDIAN, etc. won't work with unit tests
2 participants