CT 1604 remove compiled classes #6384
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. |
1 similar comment
|
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. |
|
I renamed the core/dbt/contracts/graph/parsed.py file to core/dbt/contracts/graph/nodes.py I've renamed the main "Parsed" classes: ParsedModelNode, ParsedAnalysisNode, ParsedSeedNode, ParsedSqlNode, ParsedGenericTestNode, ParsedSingularTestNode, ParsedHookNode, ParsedSeedNode, ParsedSnapshotNode, ParsedSourceDefinition, ParsedMetric, ParsedExposure and ParsedMacro to ModelNode, AnalysisNode, SeedNode, SqlNode, GenericTestNode, SingularTestNode, HookNode, SeedNode, SnapshotNode, SourceDefinition, Exposure, Metric and Macro. I left ParsedNode alone (opinions?). Changed ParsedTestNode to TestNode, ParsedResource to Resource. Had to move a few Unions from the former compiled.py file to nodes.py I'm planning to try to make a parsed.py file for compatibility with the old names to see if we can avoid some adapter updates, otherwise this will require that various adapter tests be modified. Might need to do that anyway... |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
Co-authored-by: Emily Rockman <[email protected]>
| @@ -0,0 +1,7 @@ | |||
| kind: Under the Hood | |||
| body: Remove compiled node classes | |||
There was a problem hiding this comment.
Feels more like "unify" or "consolidate" ParsedNode + CompiledNode classes
There was a problem hiding this comment.
True. Will change wording.
| @@ -32,7 +32,7 @@ class RemoteResult(VersionedSchema): | |||
| class RemoteCompileResultMixin(RemoteResult): | |||
| raw_code: str | |||
| compiled_code: str | |||
| node: CompileResultNode | |||
| node: ResultNode | |||
There was a problem hiding this comment.
This may have implications for dbt-rpc (here). I don't think this should have any direct implications for dbt-server / lib.py, but worth testing to be sure.
| compiled_code: Optional[str] = None | ||
| extra_ctes_injected: bool = False | ||
| extra_ctes: List[InjectedCTE] = field(default_factory=list) | ||
| relation_name: Optional[str] = None |
There was a problem hiding this comment.
It feels weird and incorrect that relation_name is only available during compilation, rather than after parsing. I think the reason had been, we need to know the adapter's quoting config — but I'm pretty sure we'd be able to know that at parse time too, right?
There was a problem hiding this comment.
I think so. I agree that it would be nice to have this available at parse time. I think "relation_name" was added by an external contributor, so I'm guessing it was at compile time because it was easier to find a place to put it.
There was a problem hiding this comment.
We do set "relation_name" for SourceDefinition at parse time.
There was a problem hiding this comment.
I'll look at setting relation_name in the right place in the pull request to implement the nodes in protobuf messages.
There was a problem hiding this comment.
I think you're right! Sounds good.
|
I checked the adapter repos and the only imports from a changed file are for a tests/unit/utils.py file in methods which are not actually used anywhere, so it doesn't look like the CI tests should fail. Those repos should be updated to remove the dead code though, so I'll open tickets to do that after this is merged. I don't see any imports in dbt-server from dbt.contracts.graph, so I think we're good there. dbt-rpc will have to be updated. I'll open a ticket to do that after this is merged. |
resolves #6383
Description
Remove compiled classes in order to simplify the artifact generation and code.
Checklist
changie newto create a changelog entry