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

Fix #8031: Call materialization macro from adapter dispatch #8355

Closed
wants to merge 17 commits into from

Conversation

aranke
Copy link
Member

@aranke aranke commented Aug 10, 2023

resolves #8031
docs dbt-labs/docs.getdbt.com/#

Problem

Solution

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

@aranke aranke requested a review from a team as a code owner August 10, 2023 17:34
@aranke aranke requested review from peterallenwebb, gshank and MichelleArk and removed request for peterallenwebb August 10, 2023 17:34
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #8355 (97725ef) into main (424f3d2) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    dbt-labs/dbt-core#8355      +/-   ##
==========================================
- Coverage   86.36%   86.36%   -0.01%     
==========================================
  Files         174      174              
  Lines       25575    25580       +5     
==========================================
+ Hits        22089    22093       +4     
- Misses       3486     3487       +1     
Flag Coverage Δ
integration 83.20% <100.00%> (-0.02%) ⬇️
unit 65.17% <86.11%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
core/dbt/clients/jinja.py 89.17% <100.00%> (+0.31%) ⬆️
core/dbt/context/providers.py 88.76% <100.00%> (+0.09%) ⬆️
core/dbt/task/clone.py 88.11% <100.00%> (-0.12%) ⬇️
core/dbt/task/run.py 92.74% <100.00%> (-0.03%) ⬇️

... and 1 file with indirect coverage changes

@cla-bot cla-bot bot added the cla:yes label Aug 10, 2023
@aranke
Copy link
Member Author

aranke commented Aug 10, 2023

🎩 (same behavior)

❯ dbt clone --state target/ --target snowclone --target-path target_clone --full-refresh
17:27:41  Running with dbt=1.7.0-a1
17:27:42  Registered adapter: snowflake=1.6.0
17:27:42  Found 5 models, 3 seeds, 20 tests, 0 sources, 0 exposures, 0 metrics, 374 macros, 0 groups, 0 semantic models
17:27:42  
17:27:44  Concurrency: 8 threads (target='snowclone')
17:27:44  
17:27:50  
17:27:50  Completed successfully
17:27:50  
17:27:50  Done. PASS=8 WARN=0 ERROR=0 SKIP=0 TOTAL=8

@gshank
Copy link
Contributor

gshank commented Aug 11, 2023

Pretty simple so far, and glad that it's working.

Now that those basics are in, there are a few things to look at doing... You should use the same type of context.adapter.dispatch call in the run task, and look for breakage.

@jtcohen6 was interested in allowing the same naming convention for materialization macros that we use for other macros, for ease of overriding. Right now we can't override easily. That's the naming convention that starts with the adapter, like "postgres__drop_schema". You should attempt to allow both forms of materialization naming and get them to work, with the "<adapter_name>__" style name overriding the earlier format.

I suspect that when we use context.adapter.dispatch in the run task we'll get breakage due to the special context that's used by the code that detects where the python code is being called from. You'll probably need to set that where the MacroGenerator objects are created.

@aranke
Copy link
Member Author

aranke commented Aug 15, 2023

All existing tests are passing, here are the next steps:

  • Add context_stack for Python models (and test if possible)
  • Add tests for calling materialization from macro (can be another materialization)
  • Add tests for name resolution for materializations, both old (materialization_foo_adapter) and new (adapter__materialization)

attempts.append(f"{package_name}.{search_name}")

if macro is not None:
if macro_name.startswith("materialization_"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we made a fix recently for scenarios where macros that started with "materialization_" were incorrectly being picked up as materializations. For example, this was being flagged as a materialization instead of a macro:

{% macro materialization_setup() %}
    ...
{% endmacro %}

Have you verified this doesn't reintroduce that issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have tests for that case now, so this PR shouldn't reintroduce the issue (or if it does, it'll be obvious before merging).

#8181

@gshank gshank removed their request for review September 8, 2023 14:04
@aranke
Copy link
Member Author

aranke commented Sep 13, 2023

Closing this PR per discussion with @mikealfare, creating new ticket to migrate clone to use get_replace_sql.

dbt-labs/dbt-adapters#225

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.

[CT-2790] Support dispatch for materialization macros
3 participants