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

[CT-1301] Behavior change: full_refresh: true config should respect --full-refresh, -f flag #6013

Open
jtcohen6 opened this issue Oct 6, 2022 · 7 comments
Labels
enhancement New feature or request incremental Incremental modeling with dbt Team:Adapters Issues designated for the adapter area of the code

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Oct 6, 2022

https://docs.getdbt.com/reference/resource-configs/full_refresh

Current behavior

When configured, full_refresh: true|false means: "always/never full-refresh, and always ignore the --full-refresh flag"

Added way back in #2438 / v0.18

Proposed change

  • full_refresh: true should mean "always respect the --full-refresh flag"
  • full_refresh: false should mean "never full refresh, under any circumstances"

That's trickier for intuition, and harder to explain, but definitely more desirable in terms of the behavior it enables. I'm not sure why you'd always want to full-refresh an incremental model; at that point it's really just a table materialization.

If this is set at the project level:

models:
  +full_refresh: false

It's not possible to override that setting for one particular model ("actually I want this model to respect the --full-refresh flag"). This doesn't work as I'd hoped it might:

{{ config(materialized = 'incremental', full_refresh = none) }}

Is this a breaking change?

Only for people who currently make use of full_refresh: true. Again, I'm not actually sure what the use case for doing that would be!

@jtcohen6 jtcohen6 added enhancement New feature or request incremental Incremental modeling with dbt Team:Adapters Issues designated for the adapter area of the code labels Oct 6, 2022
@github-actions github-actions bot changed the title Behavior change: full_refresh: true config should respect --full-refresh, -f flag [CT-1301] Behavior change: full_refresh: true config should respect --full-refresh, -f flag Oct 6, 2022
@b-per
Copy link
Contributor

b-per commented Oct 6, 2022

The only reason I could think of people using full_refresh: true would be from some env_var(), e.g. always doing a full-refresh in Dev (and maybe limiting the number of rows) but keeping it incremental in Prod. This looks very niche though. Most of the time that I need this parameter it is to set it to false to prevent FR.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

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

@github-actions github-actions bot added the stale Issues that have gone stale label Apr 5, 2023
@b-per
Copy link
Contributor

b-per commented Apr 5, 2023

I think that it is worth keeping this issue active

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

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

@github-actions github-actions bot added the stale Issues that have gone stale label Oct 3, 2023
@b-per
Copy link
Contributor

b-per commented Oct 3, 2023

Commenting to keep it on! I still think that we could improve incremental models handling.

@jtcohen6 jtcohen6 removed the stale Issues that have gone stale label Oct 3, 2023
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Oct 3, 2023

@b-per I agree! I think we'd want to find some way to change the default here, without breaking existing users' projects. If you can think of a clever way to do that, without requiring another opt-in config (full_refresh_config_respect_flag: true), I'm all ears.

This behavior would then also diverge from --store-failures / store_failures, where the node-level config always takes precedence over the flag. Should we change that behavior too, to match?

@andrii0yerko
Copy link

I had faced the same problem.

Did anyone find any workarounds to achieve respect for the --full-refresh flag?

I tried things like

{{
    config(
        materialized='incremental',
        full_refresh = flags.FULL_REFRESH,
        ...
}}

but it doesn't work. Seems that full_refresh cannot be set dynamically for some reasons

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request incremental Incremental modeling with dbt Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

No branches or pull requests

3 participants