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-254] dbt-core - Snapshot - macro dbt_macro__create_schema takes not more than 1 argument(s) #4742

Closed
rlshuhart opened this issue Feb 17, 2022 · 3 comments · Fixed by #4993
Labels
bug Something isn't working Team:Adapters Issues designated for the adapter area of the code
Milestone

Comments

@rlshuhart
Copy link

{% if not adapter.check_schema_exists(model.database, model.schema) %}
{% do create_schema(model.database, model.schema) %}
{% endif %}

Hi I've raise this same issue in dbt-sqlserver, but the issue might actually lie here after further investigation. I'm running into the issue, "macro 'dbt_macro__create_schema' takes not more than 1 argument(s)", when attempting to do a simple snapshot. When I delete lines 9-11 in the above all goes well.

Please see additional details on dbt-sqlserver issue at dbt-msft/dbt-sqlserver#208

@github-actions github-actions bot changed the title dbt-core - Snapshot - macro 'dbt_macro__create_schema' takes not more than 1 argument(s) [CT-254] dbt-core - Snapshot - macro dbt_macro__create_schema takes not more than 1 argument(s) Feb 17, 2022
@jtcohen6 jtcohen6 added bug Something isn't working Team:Adapters Issues designated for the adapter area of the code triage labels Feb 17, 2022
@jtcohen6 jtcohen6 removed the triage label Mar 18, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Mar 18, 2022

@rlshuhart Thanks for opening, you're totally right! We need to make that create_schema call take a single Relation argument, rather than database + schema. This was a breaking change a long time ago... all the way back in v0.17 (#2411).

It's worth checking to see if we actually need this create_schema call at all. I have a sneaking suspicion that the snapshot may already be properly accounted for by this logic:

def before_run(self, adapter, selected_uids: AbstractSet[str]):
with adapter.connection_named("master"):
self.create_schemas(adapter, selected_uids)

If that's indeed the case, your solution (deleting lines 9-11) is the right one.

@jtcohen6 jtcohen6 added this to the v1.1 milestone Mar 18, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 4, 2022

I've concluded these lines are totally unneeded. After a bit of digging through the git blame, I see that the origin of the create_schema in the snapshot (then archive) materialization was in: #625

At the time, Drew noted that:

There might be a smarter way to do this -- we could set the node schema to the value of target_schema, then this will happen automatically through the node_runner before_run flow.

This is in fact what we do:

# the target schema must be set if we got here, so overwrite the node's
# schema
node.schema = node.config.target_schema

And have been doing since #1478 (here specifically). This is picked up accordingly by the before_run method in the snapshot task (inherited from the method in the run task shown above).

Worth noting that this piece still isn't perfect, if you're defining target_schema in a .yml file instead of snapshots/*.sql: #4000

@rlshuhart
Copy link
Author

Thanks a lot for looking into this, @jtcohen6!

@jtcohen6 jtcohen6 removed their assignment Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants