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 inheritance to materialization macro resolution #5348

Conversation

volkangurel
Copy link
Contributor

@volkangurel volkangurel commented Jun 8, 2022

addresses parts of #4646

Description

Adapter inheritance currently works for macro resolution but doesn't work for materialization macro resolution. This PR updates the logic for the materialization macro resolution to use inheritance as well.

Checklist

@volkangurel volkangurel requested review from a team as code owners June 8, 2022 14:42
@volkangurel volkangurel requested review from emmyoop and gshank June 8, 2022 14:42
@cla-bot cla-bot bot added the cla:yes label Jun 8, 2022
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.

Thanks so much for picking this up @volkangurel!

Even though we're not achieving full consistency (between macro dispatch / materialization candidacy), as proposed in #4646, it is a step on the way there, and a big benefit to adapter developers in the meantime.

I've tagged this for review by both Team:Language + Team:Adapters, since it touches code relevant to both. Sign-off from one or the other is probably enough to see this in for v1.2.

Comment on lines +670 to +674
# order matters for dispatch:
# 1. current adapter
# 2. any parent adapters (dependencies)
# 3. 'default'
return get_adapter_type_names(adapter_type) + ["default"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This order looks right to me. Thanks for the clear annotation!

Question: Would we be better served by incorporating actual dispatch logic here, rather than reimplementing it? I get that materialization macros are a special case (using a different / older naming structure), so there may not be a way to do this without expanding the scope of this PR significantly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'm not very familiar with how providers work and how they can be injected into the manifest class instance here. But the approach in this PR doesn't work I'm happy to go down that path with some guidance from others.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this approach. However, @volkangurel could you leave a comment here to this effect? Something that notes that this is duplicated logic and could instead be incorporating actual dispatch logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment here.

@jtcohen6 jtcohen6 added Team:Adapters Issues designated for the adapter area of the code ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Jun 14, 2022
@nathaniel-may nathaniel-may requested review from a team and VersusFacit June 15, 2022 19:24
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Thanks for the work here, @volkangurel! @McKnight-42 and I just went though the changes, and it looks pretty good to us. We just want one additional set of tests, and some comments. Otherwise this looks good to merge.

expected=(project, 'foo'),
) for project in ['root', 'dep', 'dbt']
)

Copy link
Contributor

Choose a reason for hiding this comment

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

This additional set of test inputs looks good. I also want to check that if macros for both foo and bar are present, bar gets chosen. Maybe something like this:

    sets.extend(
        FindMaterializationSpec(
            macros=[MockMaterialization(project, adapter_type='foo'), MockMaterialization(project, adapter_type='bar')],
            adapter_type='bar',
            expected=(project, 'bar'),
        ) for project in ['root', 'dep', 'dbt']
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I added this test.

Comment on lines +670 to +674
# order matters for dispatch:
# 1. current adapter
# 2. any parent adapters (dependencies)
# 3. 'default'
return get_adapter_type_names(adapter_type) + ["default"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this approach. However, @volkangurel could you leave a comment here to this effect? Something that notes that this is duplicated logic and could instead be incorporating actual dispatch logic.

@@ -355,12 +350,10 @@ def __lt__(self, other: object) -> bool:

@dataclass
class MaterializationCandidate(MacroCandidate):
specificity: Specificity
specificity: int
Copy link
Contributor

Choose a reason for hiding this comment

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

@volkangurel could you leave a comment on this parameter describing what the int value represents and how it should be set? The previous Specificity class had names that helped with this, but int isn't obvious how it is used by the MaterializationCandidate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

def _materialization_candidates_for(
self,
project_name: str,
materialization_name: str,
adapter_type: Optional[str],
adapter_type: str,
specificity: int,
Copy link
Contributor

Choose a reason for hiding this comment

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

@volkangurel I might also leave that specificity comment here since this seems to be where it is passed in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@nathaniel-may nathaniel-may added awaiting_response and removed ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Jun 23, 2022
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments! It LGTM: you're free to merge.

@gshank gshank merged commit 3500528 into dbt-labs:main Jul 6, 2022
@volkangurel volkangurel deleted the add-inheritance-to-materialization-resolution branch July 6, 2022 18:41
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
* Add inheritance to materialization macro resolution

* Add changelog entry

* Address PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_response cla:yes ok_to_test Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants