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 duplicate refable node names across packages #7374

Merged
merged 11 commits into from
May 8, 2023

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Apr 17, 2023

resolves: #7446
resolves: #1269

Description

  • removes DuplicateResourceNameError call in manifest parsing
  • uses _find_unique_ids_for_package in RefableLookup
  • raises AmbiguousResourceNameRefError in RefableLookup.get_unique_id if multiple unique_ids found
  • plumbs caller node (source_node) through RefableLookup for node-aware CompilationError message

Checklist

Compilation Error in model model_downstream (models/model_downstream.sql)
  dbt found multiple potential nodes when referencing "model" - ['model.project_b.model', 'model.project_a.model'].
  To fix this, specify which package to use as the first argument to 'ref' (one of: ['project_b', 'project_a']).

…se AmbiguousResourceNameError in find_unique_id_for_package
@cla-bot cla-bot bot added the cla:yes label Apr 17, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@MichelleArk MichelleArk changed the title [POC] Allow duplicate model names across packages Allow duplicate refable node names across packages Apr 29, 2023
@MichelleArk MichelleArk force-pushed the poc/namespaced-refs branch from 26d48d4 to 1c3b5b3 Compare April 29, 2023 20:21
@MichelleArk MichelleArk marked this pull request as ready for review May 1, 2023 02:03
@MichelleArk MichelleArk requested review from a team as code owners May 1, 2023 02:03
@MichelleArk MichelleArk requested review from QMalcolm, aranke, VersusFacit, gshank, peterallenwebb and emmyoop and removed request for a team, QMalcolm, aranke and VersusFacit May 1, 2023 02:03
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Looks good! It's actually surprising that it didn't take more than this.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

nice!! took it for a spin, & it spins

shall we queue up #1269 to close? :)

core/dbt/exceptions.py Outdated Show resolved Hide resolved
@MichelleArk MichelleArk closed this May 5, 2023
@MichelleArk MichelleArk reopened this May 5, 2023
@jtcohen6
Copy link
Contributor

jtcohen6 commented May 6, 2023

Ah, something that's still not working: attaching yaml properties to the correct node in the case of duplicates. (This was also the blocker to merging #3053 a few years ago.)

# packages.yml
packages:
  - local: package_a
  - local: package_b
-- package_a/models/my_model.sql
select 1 as id
# package_a/models/schema.yml
models:
  - name: my_model
    description: My cool model
-- package_b/models/my_model.sql
{{ config(alias="something_else") }}
select 1 as id
-- models/another_model.sql
select * from {{ ref('package_a', 'my_model') }}
$ dbt run
06:40:32  Running with dbt=1.6.0-a1
...
Compilation Error
  When referencing 'my_model', dbt found nodes in multiple packages: 'model.package_b.my_model', 'model.package_a.my_model'
  To fix this, use two-argument 'ref', with the package name first: 'package_b' or 'package_a'

Goal: It should be possible to define yaml properties for a model within the same package as that model.

(In the future, we could look to support "overrides" of the package resource from the root project, similar to how we do with sources. That's definitely out of scope for now.)

@MichelleArk MichelleArk force-pushed the poc/namespaced-refs branch from ec62766 to 1fa3608 Compare May 7, 2023 17:36
@MichelleArk
Copy link
Contributor Author

@jtcohen6 - Pushed up a fix for defining yaml properties on a model within the same package as that model.

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-2472] Allow duplicate model names across packages Add namespacing for dbt resources
3 participants