remove manifest from adapter.set_relations_cache signature #9213
Conversation
|
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. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## relation-create-from-refactor #9213 +/- ##
=================================================================
+ Coverage 86.56% 86.61% +0.05%
=================================================================
Files 215 215
Lines 26862 26866 +4
=================================================================
+ Hits 23252 23271 +19
+ Misses 3610 3595 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
MichelleArk
requested review from
mikealfare,
peterallenwebb and
colin-rogers-dbt
and removed request for
peterallenwebb and
mikealfare
| @@ -406,11 +406,21 @@ def populate_adapter_cache( | |||
| if not self.args.populate_cache: | |||
| return | |||
|
|
|||
| if self.manifest is None: | |||
| raise DbtInternalError("manifest was None in populate_adapter_cache") | |||
There was a problem hiding this comment.
Any idea under what conditions we wouldn't have a manifest?
There was a problem hiding this comment.
This was primarily added out of defensiveness & mypy appeasement , and is not an real expected condition -- it would be our own improper usage of populate_adapter_cache. Since GraphRunnableTask inherits from ConfiguredTask which has an optional manifest, mypy was flagging the None-value of manifest as potentially problematic. the manifest gets set earlier on during GraphRunnableTask execution and should not be None by this point.
| node | ||
| for node in self.manifest.nodes.values() | ||
| if (node.is_relational and not node.is_ephemeral_model and not node.is_external_node) | ||
| ] |
There was a problem hiding this comment.
any advantage to having this be a method or just a calculated attribute of the manifest itself?
There was a problem hiding this comment.
discussed sync: there isn't a great place for this logic -- the manifest shouldn't really need to know about the 'cacheable' concept, and it's only used in a single place. Leaving as-is for simplicity for the time being.
|
closing in favor of: #9242 |
resolves #9217
Problem
Setting the relations cache requires an entire manifest currently, even though just a sliver of functionality is necessary for the task.
Solution
Pre-compute the manifest nodes to populate the cache for - passing in an iterable of RelationConfigs to the adapter instead of the entire manifest.
Checklist