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

Add behavior change flag docs for adapter maintainers #6092

Merged
merged 6 commits into from
Sep 19, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions website/docs/guides/adapter-creation.md
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,106 @@ While much of dbt's adapter-specific functionality can be modified in adapter ma

See [this GitHub discussion](https://github.com/dbt-labs/dbt-core/discussions/5468) for information on the macros required for `GRANT` statements:

### Behavior change flags

Starting in `dbt-adapters==1.5.0` and `dbt-core==1.8.7`, adapter maintainers have the ability to implement their own behavior change flags.
For more information on what a behavior change is, please refer to [Behavior changes](https://docs.getdbt.com/reference/global-configs/behavior-changes).
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
To implement a behavior change flag, provide a name, a default setting (`True` / `False`), and optional source, and either a description or a link to the flag's documentation on docs.getdbt.com.
Copy link
Collaborator

@amychen1776 amychen1776 Sep 18, 2024

Choose a reason for hiding this comment

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

Suggested change
To implement a behavior change flag, provide a name, a default setting (`True` / `False`), and optional source, and either a description or a link to the flag's documentation on docs.getdbt.com.
To implement a behavior change flag, you will need to provide a name for the flag, a default setting (`True` / `False`), optional source, and a description and/or a link to the flag's documentation on docs.getdbt.com. We recommend having a description and documentation link whenever possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also what is source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

source is basically a namespace. I would list the adapter as the source for adapter behavior flags. But the framework can be used elsewhere, so I don't force that. I also provide a default that takes the calling module, so that even if the maintainer does not add a source, there's still something that we can use to differentiate flags with the same name across multiple adapters (or not in an adapter at all). I wanted to try and automate it since it would feel silly to need to specify the adapter for every flag. We could probably be smarter than this and set it in BaseAdapter somehow when they get registered.

The description and/or docs should provide end users with context for why the flag exists, why they may see a warning, and why they may want to override the default.
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
Behavior change flags can be implemented by overwriting `_behavior_flags()` on the adapter in `impl.py`:

<File name='impl.py'>

```python
class ABCAdapter(BaseAdapter):
...
@property
def _behavior_flags(self) -> List[BehaviorFlag]:
return [
{
"name": "enable_new_functionality_requiring_higher_permissions",
"default": False,
"source": "dbt-abc",
"description": (
"The dbt-abc adapter is implementing a new method for sourcing metadata. "
"While we feel this is a better way to source metadata, it does require higher permissions on the platform. "
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
"Enabling this without granting the requisite permissions will result in an error. "
"This feature is expected to be required by Spring 2025."
),
"docs_url": "https://docs.getdbt.com/reference/global-configs/behavior-changes#abc-enable_new_functionality_requiring_higher_permissions",
}
]
```

</File>

Once a behavior change flag has been implemented, it can be referenced on the adapter both in `impl.py` and in jinja macros:

<File name='impl.py'>

```python
class ABCAdapter(BaseAdapter):
...
def some_method(self, *args, **kwargs):
if self.behavior.enable_new_functionality_requiring_higher_permissions:
# do the new thing
else:
# do the old thing
```

</File>

<File name='adapters.sql'>

```sql
{% macro some_macro(**kwargs) %}
{% if adapter.behavior.enable_new_functionality_requiring_higher_permissions %}
{# do the new thing #}
{% else %}
{# do the old thing #}
{% endif %}
{% endmacro %}
```

</File>

Every time the behavior flag evaluates to `False`, it will fire a warning to the user informing them that there will be a future change.
This warning doesn't fire when the flag evaluates to `True` as the user is already in the new experience.
The warnings can be noisy, and isn't always desired. To evaluate the flag without firing the warning, append `.no_warn` to the end of the flag:

<File name='impl.py'>

```python
class ABCAdapter(BaseAdapter):
...
def some_method(self, *args, **kwargs):
if self.behavior.enable_new_functionality_requiring_higher_permissions.no_warn:
# do the new thing
else:
# do the old thing
```

</File>

<File name='adapters.sql'>

```sql
{% macro some_macro(**kwargs) %}
{% if adapter.behavior.enable_new_functionality_requiring_higher_permissions.no_warn %}
{# do the new thing #}
{% else %}
{# do the old thing #}
{% endif %}
{% endmacro %}
```

</File>

It's best practice to evaluate a behavior flag as few times as possible. This will make it easier to remove once the behavior change has matured.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's best practice to evaluate a behavior flag as few times as possible.

what does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't pepper the code with a bunch individual checks like it's a configuration. It should truly represent "a legacy way" and "a novel way".

The dbt-redshift example is a good one. Depending on whether we use the SDK or not, we either call the macro, or we call the SDK. We check the flag once.

In more complicated scenarios this may not be possible. Take Iceberg support for example. We need to check the flag in list relations and we also need to check it when creating a table in the create table macro. So we need to check it twice, there's really no way around that. But let's look at the create table macro in particular. We should just have two versions of that macro, the legacy version without Iceberg and the novel way with Iceberg; these are gated by the flag. That's good.

Using two completely different versions of the code will result in a lot of duplicated code though. Both versions need a name, they need to put the sql at the end, they have shared options like warehouse, etc. But there are multiple points where they differ as well. There are different config options like external_volume. One uses create table my_table as... and the other uses create iceberg table my_table as.... So there will be an intuitive push towards DRY code where you can embed a check inside the macro at each point where it differs. That's not good. The behavior flag is becoming part of the code, and can more easily contribute to bugs in the code, in particular in the legacy code which was supposed to be gated from this whole change in the first place.

As a result, it tends to be easier to evaluate the flag earlier in logic flow, then take either the old path or the new path.
While this may create some duplication in code, using behavior flags in this way provides a safer way to implement a change
which we are already admitting is risky or even breaking in nature.

### Other files

#### `profile_template.yml`
Expand Down
Loading