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

Allow configuring snapshot column names #1097

Merged
merged 11 commits into from
Sep 20, 2024
Merged

Allow configuring snapshot column names #1097

merged 11 commits into from
Sep 20, 2024

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Sep 3, 2024

resolves #1096

Problem

Support for dbt-labs/dbt-core#10608, configuring snapshot column names

Solution

Modify snapshot.sql to support changed column names.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@gshank gshank requested a review from a team as a code owner September 3, 2024 20:12
@cla-bot cla-bot bot added the cla:yes label Sep 3, 2024
Copy link
Contributor

@mikealfare mikealfare left a comment

Choose a reason for hiding this comment

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

Two points of clarification, otherwise this looks good.

@@ -126,7 +126,7 @@
{{ run_hooks(pre_hooks, inside_transaction=True) }}

{% set strategy_macro = strategy_dispatch(strategy_name) %}
{% set strategy = strategy_macro(model, "snapshotted_data", "source_data", config, target_relation_exists) %}
{% set strategy = strategy_macro(model, "snapshotted_data", "source_data", model['config'], target_relation_exists) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these are the same config. There's config in the globals, and then there's also config.model.config. Was there a bug or an issue that required this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'm not sure why somebody was using the passed in config to start with (I suspect it was because using defaults is different for the config object and the config dictionary that got passed in), but you can't get a "property" from the dictionary, only the config object that you get from the context.

@@ -2,7 +2,7 @@
# TODO: how to automate switching from develop to version branches?
git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core
git+https://github.com/dbt-labs/dbt-common.git
git+https://github.com/dbt-labs/dbt-adapters.git
git+https://github.com/dbt-labs/dbt-adapters.git@snapshot_column_names
Copy link
Contributor

Choose a reason for hiding this comment

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

Friendly reminder to swap back prior to merging.

{{ adapter.valid_snapshot_target(target_relation) }}
{% set stcn = config.get("snapshot_table_column_names") or get_snapshot_table_column_names() %}

{{ adapter.valid_snapshot_target(target_relation, stcn) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling adapter.valid_snapshot_target with the new second parameter will break unless we use the new version of dbt-adapters. We need to bump the minor on dbt-adapters and then bump the pin in dbt-spark to this new minor.

@gshank gshank merged commit 8c6fcb4 into main Sep 20, 2024
22 of 33 checks passed
@gshank gshank deleted the snapshot_column_names branch September 20, 2024 20:31
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.

Allow configuring snapshot column names
4 participants