Feature: nullable error_after in source

[kadero]

resolves #3874

Description

This PR aims to allow overriding the FreshnessThreshold params (error_after and warning_after).

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.

[kadero]

Hello @jtcohen6 @lchoward 👋,

Here is a first proposition based on what we agreed on the initial issue.

For now, the changes reflect what we agreed here, and the tests were fixed naively.

Before going further and adding new tests (units and integrations), I would like your thought on the current implementation

Many thanks 🙏

[jtcohen6]

@kadero Thanks so much for the contribution! I do think it would be good to add at least one end-to-end test case in test/integration/042_sources_test/, wherein a child source table disables the error_after set by its parent source.

@leahwicz The logic in this PR follows an implementation I described over in #3874 (comment). I think it would be right to have an engineer from the Core team take a look, to make sure there isn't anything big that @kadero and I are both missing.

[leahwicz]

@gshank @nathaniel-may this falls into your domain so would you be able to take a look at this please and see what you think?

[nathaniel-may]

The Python looks good to me and I only had one minor suggestion. I'm not deeply familiar with this corner of the code base yet so I'm going to withhold an explicit approval for now. Happy to do a more time consuming deep dive if we need to though.

[jtcohen6]

@kadero When you get a chance:

• Would you be able to add a test case that reproduces the desired behavior in For source freshness, allow null error_after with non-null warning_after #3874 (i.e. overrides/nulls out error_after only)?
• Could you also add a note to the changelog (topmost section), and add yourself to the list of contributors there?

Thanks again for the contribution!

[kadero]

@kadero When you get a chance:

• Would you be able to add a test case that reproduces the desired behavior in For source freshness, allow null error_after with non-null warning_after #3874 (i.e. overrides/nulls out error_after only)?
• Could you also add a note to the changelog (topmost section), and add yourself to the list of contributors there?

Thanks again for the contribution!

Hello @jtcohen6 @nathaniel-may 👋,

As we agreed, I created an integration test under test/integration/042_sources_test/, where a child source (called source_c) disables the error_after set by its parent source. 🙂

Unfortunately, the test didn't pass, everything goes on as if the child source inherit the error_after set by its parent 😕.

Do you have any clues, are we missing something?

Many thanks your support 🙏

[kadero]

return 'error_after': {'count': 24, 'period': 'hour'} instead of 'error_after': None, 😕

[kadero]

return error instead of warn
https://github.com/dbt-labs/dbt-core/runs/4000597793?check_suite_focus=true#step:9:976

[jtcohen6]

Thanks for adding the tests, and glad we caught the unexpected behavior! I think I see the place in the logic where that's occurring

[jtcohen6]

• ✅ If base and update, merge them. This will apply if update is {}, too
• ✅ If base is None, take update
• ❌ If update is None (override!), we need to return None

So something like:

def merge_freshness_time_thresholds(
    base: Optional[Time], update: Optional[Time]
) -> Optional[Time]:
    if base and update:
        return base.merged(update)
    elif update is None:
        return None
    else:
        return update or base

[kadero]

Indeed 🙈,

Thanks @jtcohen6, update done and the test is fixed ✔️

[jtcohen6]

Looks great @kadero! Thank you for the contribution :)

I just moved up the changelog entry, since we released v1.0.0b2 yesterday. As soon as the tests pass, I'll merge this in