[CT-1519] Adding friendlier error message when dist is improperly set as a list or mapping

[dluftspring]

resolves #224

Description

This PR adds a type check to the redshift adapters.sql dist macro and raises a compiler error if the user tries to set the single valued property as either a mapping or a list.

Checklist

• I have read the contributing guide and understand what's expected of me
• 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 opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[dluftspring]

What's the best way to run integration tests against Redshift? It's a straightforward change but I don't think it can be tested on postgres

[Fleid]

@dbeatty10 do you know what can be done here?
@dluftspring you can still mark the PR ready-for-review without tests, we'll add them during review (but I know it feels better to submit the whole thing together ;))

[dbeatty10]

Thanks for tagging me @Fleid 🙌

@dluftspring Yes, the configuration is specific to Redshift and wouldn't make sense against Postgres 👍 The integration tests are located in the tests/ directory, and include a suite of Redshift-specific tests and overrides.

There are two ways to run the integration tests against Redshift:

1. You can run the tests locally on your own machine by following the instructions in the CONTRIBUTING file in the dbt-redshift repository. This requires you to have access to a Redshift cluster and to set the appropriate environment variables as described in the instructions.

2. Alternatively, you can rely on GitHub CI to run the integration tests for you*. When you push new commits to your remote branch, GitHub will automatically run the CI tests for you. This can be useful if you do not have access to a Redshift cluster.

*One caveat to note is that first-time contributors to the dbt-redshift project will need a maintainer to approve their workflows in order for the CI tests to be run. This is to prevent malicious or unintended changes from being made to the project. Once your workflows have been approved by a maintainer, the CI tests will be automatically run whenever you push new commits to your branch.

To get the party started, I'm going to click this button momentarily:

In terms of creating a test that covers your case, I tried to look for a quick example of testing a compiler error. Here's a test that verifies a CompilationException is raised, and it uses the YAML configuration defined here.

Writing new integration tests can be tricky sometimes, and we're happy to help with the details if needed!

[dluftspring]

@Fleid potentially stupid question 😅 - how do I mark the PR ready for review? (apologies if that's in the contributing guide and I missed it)

[dbeatty10]

how do I mark the PR ready for review?

@dluftspring -- that's a good question, and I think you're already there!

On GitHub, a pull request can be in one of several states. The relevant ones for this discussion are:

• Draft: This represents a work in progress and will let the reviewers know that it is not yet ready for review. The maintainers generally won't comment or provide a review unless explicitly requested by the contributor.
• Open: Lets the repository maintainers know that it is ready to be looked at.

For pull requests in the draft state, you can use the "Ready for Review" button in the pull request interface:

It looks like you did this, so you're good to go! 👍

Additionally, I just now applied a ready_for_review label so the reviewer knows the associated issue has already been "approved", and they can focus on the implementation details of the pull request:

(We treat this label as a "private" one reserved for maintainers to add/remove rather that one a contributor should apply.)

[VersusFacit]

I have to figure out why adapter tests aren't running and I suspect it's because of a path exception that is out of date and needs to be changed on Main. I have to get back here at some point soon

[mikealfare]

I think this needs a do in front of it, e.g.:

{% do exceptions.raise_compiler_error("Expected a single valued property for dist and got: " ~ dist ~ " instead") %}

Alternatively, I think {{ instead of {% also works:

{{ exceptions.raise_compiler_error("Expected a single valued property for dist and got: " ~ dist ~ " instead") }}

Otherwise jinja is looking for a keyword like set, if, macro, etc.

[dluftspring]

Yeah that's my mistake on the compiler error- it needs a do statement. Let me update this later today. There's an issue with the conditional check. Strings are technically iterable so this isn't doing what I want it to do in any case

[dbeatty10]

@dluftspring I didn't research the possible reasons why, but there are a handful of tests that are failing CI checks:

FAILED tests/functional/adapter/test_simple_seed.py::TestSimpleSeedDiststyleAll::test_simple_seed_with_diststyle_all - AssertionError: dbt exit state did not match expected
FAILED tests/functional/adapter/materialized_view_tests/test_materialized_views.py::TestRedshiftMaterializedViewChangesApply::test_change_is_applied_via_alter - AssertionError: dbt exit state did not match expected
ERROR tests/functional/adapter/backup_tests/test_backup_table.py::TestBackupTableSyntax::test_backup_predicate_precedes_secondary_predicates[syntax_with_distkey-diststyle key distkey] - AssertionError: dbt exit state did not match expected
ERROR tests/functional/adapter/backup_tests/test_backup_table.py::TestBackupTableSyntax::test_backup_predicate_precedes_secondary_predicates[syntax_with_sortkey-compound sortkey] - AssertionError: dbt exit state did not match expected

[mikealfare]

You can ignore the materialized view test. That's a known flaky test. But the other three are consistently failing across python version and OS.

[github-actions]

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

[github-actions]

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.