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

set default materialized for test node configs #2902

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

kwigley
Copy link
Contributor

@kwigley kwigley commented Nov 19, 2020

(possibly) resolves #2806

Description

After noticing similar behavior for seed and snapshot node configs, this change sets the default materialized for test node configs to test. It aligns selection behavior, but this is definitely extending the meaning of materialized.

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

@cla-bot cla-bot bot added the cla:yes label Nov 19, 2020
@gshank
Copy link
Contributor

gshank commented Nov 19, 2020

Looks like a great easy win!

@rsella
Copy link
Contributor

rsella commented Nov 20, 2020

Just adding my two cents here, feel free to ignore if I missed something in the big picture.

In my opinion materialized = test can make great sense for data tests, even if it means extending the "formal" materialized meaning (as noted by @kwigley )

For schema test I personally think that materialized should be set to the model materialization.
So for example if the model has incremental, it should be incremental, if model has view it should be view, and so on

I can see two main benefits:

  1. Selecting models with a filter on materialization (config.materialized: view|table|...) will actually also select the related schema tests. Using the materialized = test, I guess we will lose a flexible way of selecting things
  2. While I can get a sense of "materialization" for data test, I fail to see a meaning in materialization for schema tests (what is materialization for a IS NOT NULL check?).
    In my idea the materialization for schema test will be then intended as a "copy" of the parent (target) model

I don't know if it is possible to do so, I don't have enough knowledge of the codebase

What do you think?

@jtcohen6
Copy link
Contributor

jtcohen6 commented Nov 20, 2020

@rsella Appreciate you weighing in! A lot of our thinking for the release after the next is around tests: how can we rationalize them, make them a bit more fully-featured, and reconcile confusing differences between data tests and schema tests.

Per the proposal in #2593, we want a bit more heft to tests such that they can wrap SQL (either from data test file or schema test macro), apply a common set of configs (severity, limit), calculate the number of failures, and optionally write those failing results to a table. This PR has helped me realize that the right wrapper may in fact be a formal Jinja materialization, named test, which could then be reimplemented on different adapters and in users' own projects as needed.

Selecting models with a filter on materialization (config.materialized: view|table|...) will actually also select the related schema tests. Using the materialized = test, I guess we will lose a flexible way of selecting things

This already works, based on the way that test selection works today—dbt test takes account of properties of parent models when identifying tests. The specific bug we're trying to resolve here is that, today in v0.18:
dbt test -m config.materialized:table = run tests that select from table-materialized models
dbt test -m config.materialized:view = run all tests (because every test has the default value of materialized: 'view')

With this code change:
dbt test -m config.materialized:table = run tests that select from table-materialized models
dbt test -m config.materialized:view = run tests that select from view-materialized models
dbt test -m config.materialized:test = run all tests = dbt test

@rsella
Copy link
Contributor

rsella commented Nov 20, 2020

@jtcohen6 all clear, seems like the materialized = test proposal is perfect then

Thanks for the explanation btw

@kwigley kwigley self-assigned this Nov 23, 2020
@kwigley kwigley marked this pull request as ready for review November 23, 2020 15:37
@kwigley kwigley changed the title (wip) set default materialized for test node configs set default materialized for test node configs Nov 23, 2020
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Let's do it.

This PR has made me realize that it's possible to do things like:

seeds:
  +materialized: view

This results in a database error, as dbt tries to wrap a lot of missing information in a create view statement. Or:

seeds:
  +materialized: asdfghjkl

Which does raise a compilation error, but perhaps not the one you'd expect:

No materialization 'asdfghjkl' was found for adapter postgres! (searched types 'default' and 'postgres')

I don't know if we should consider that user error and do nothing, or actively raise exceptions when users set the materialized config for non-models. In any case, it's an ongoing question and outside the scope of this PR to fix.

@kwigley kwigley requested a review from gshank November 24, 2020 15:02
@kwigley kwigley merged commit fec0e31 into dev/kiyoshi-kuromiya Nov 24, 2020
@kwigley kwigley deleted the fix/test-selection branch November 24, 2020 17:19
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.

dbt test [--models|--exclude] config.materialized:view does not seem to work
4 participants