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

fix catalog generation #808

Merged
merged 5 commits into from
Jul 3, 2018
Merged

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Jun 23, 2018

Fixes some issues around catalog generation/operations

Changes:

  • Do not add this to the context for Operations
  • Only add operations to the graph once. Previously, they were added once a Macro node, and once as an Operation node. Similarly, Operation nodes were previously created for code declared in {% macro %} blocks.
  • Fix missing parent_map and child_map entries. This happened because the manifest was written out before refs were processed
  • Run the compilation step before writing out the catalog for dbt docs generate. This simplifies the code a whole lot

"model": node,
"sql": node.get('injected_sql'),
"this": get_this_relation(db_wrapper, project_cfg, profile, node),
"ref": provider.ref(db_wrapper, node, project_cfg, profile, flat_graph)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref, sql should be in the global context

@@ -17,7 +17,9 @@ def load_all(cls, root_project, all_projects):
for loader in cls._LOADERS:
nodes.update(loader.load_all(root_project, all_projects, macros))

return ParsedManifest(nodes=nodes, macros=macros)
manifest = ParsedManifest(nodes=nodes, macros=macros)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tell the ParsedManifest to process itself:

  • process refs
  • incorporate Schema Spec info

UnparsedManifest.parse() ---> ParsedManifest

Copy link
Member

Choose a reason for hiding this comment

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

i've started an issue for this here: #821

@cmcarthur
Copy link
Member

given the changes on this plus what i sent you about parsing, maybe we should hold off on this. it's not actually causing problems rn, right?

@drewbanin
Copy link
Contributor Author

@cmcarthur yeah, I had to do this because of operations for catalog generation. Specifically, the code that gets this for an operation fails. We could probably hack around it by checking the node type if you're cool with that

@cmcarthur
Copy link
Member

@drewbanin ok, yeah that sounds fine

@drewbanin
Copy link
Contributor Author

drewbanin commented Jul 2, 2018

@cmcarthur I made some other changes around operations in this PR. Going to rip them out and revise this PRs description

@cmcarthur cmcarthur changed the title separate context for operations fix catalog generation Jul 3, 2018
@drewbanin drewbanin assigned drewbanin and unassigned cmcarthur Jul 3, 2018
@drewbanin drewbanin merged commit 89e45fb into development Jul 3, 2018
@drewbanin drewbanin deleted the faeture/improve-catalog-generation branch July 16, 2018 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants