Operation result #774
Operation result #774
Conversation
…esult type for operations
dbt/adapters/default/impl.py
Outdated
|
|
||
| # This is a lot of magic, have to know the magic name is 'catalog'. | ||
| # TODO: How can we make this part of the data set? Could we make it | ||
| # the operation's name/unique ID somehow instead? |
dbt/adapters/default/impl.py
Outdated
|
|
||
| # TODO: should I get the return value here in case future operations | ||
| # want to return some string? Jinja (I think) stringifies the results | ||
| # so it's not super useful. Status, I guess? |
There was a problem hiding this comment.
no, i think this is fine as is. can you remove this comment?
dbt/adapters/default/impl.py
Outdated
| Return an an AttrDict with three attributes: 'table', 'data', and | ||
| 'status'. 'table' is an agate.Table. | ||
| """ | ||
| operation = manifest.find_operation_by_name(operation_name, None) |
There was a problem hiding this comment.
i believe you can pass "dbt" as the package name here instead of None
dbt/adapters/default/impl.py
Outdated
| # Abstract methods involving the flat graph | ||
| ### | ||
| @classmethod | ||
| def get_catalog(cls, profile, project_cfg, run_operation): |
There was a problem hiding this comment.
there's some naming inconsistency here. the comment should read manifest instead of flat graph, run_operation should be manifest to be consistent with the underlying implementations
|
|
||
| def find_operation_by_name(self, name, package): | ||
| return self._find_by_name(name, package, 'macros', | ||
| [NodeType.Operation]) |
dbt/main.py
Outdated
| catalog_sub = subs.add_parser('catalog', parents=[base_subparser]) | ||
| catalog_subs = catalog_sub.add_subparsers() | ||
| # it might look like catalog_sub is the correct parents entry, but that | ||
| # will cause weird errors about 'conflicting option strings'. |
There was a problem hiding this comment.
@drewbanin should this be docs? dbt catalog generate vs dbt docs generate?
There was a problem hiding this comment.
Yeah, I was thinking dbt docs generate here. Might be worth changing the GenerateTask in generate.py to DocsTask and docs.py?
There was a problem hiding this comment.
Yup, I'll fix the catalog -> docs.
Is this actually a "docs task"? I assume the point of having generate be a subcommand of docs is that there are other docs tasks planned, right? If not, maybe this could just be dbt docs?
dbt/node_runners.py
Outdated
|
|
||
| @property | ||
| def skipped(self): | ||
| return self.skip |
There was a problem hiding this comment.
it looks to me like you don't need to override errored, failed, or skipped
dbt/task/generate.py
Outdated
| return structured | ||
|
|
||
|
|
||
| # derive from BaseTask as I don't really want any result interpretation. |
There was a problem hiding this comment.
this comment isn't necessary, if you feel that this is confusing, you should add docstrings to BaseTask and RunnableTask explaining the differences
| # project (I think?) - it assumes that you have an existing and | ||
| # populated project['modules-path'], but 'catalog generate' shouldn't | ||
| # require that. It might be better to suppress the exception in | ||
| # dependency_projects if that's reasonable, or make it a flag. |
There was a problem hiding this comment.
when you say existing and populated project['modules-path'], does that mean that the directory exists, and that dbt deps has been run to pull in the project's dependencies?
imo, dbt deps should be run before dbt catalog generate -- I can't imagine this working any other way
There was a problem hiding this comment.
Yeah. As it is, this works in the sample talk project I have without having to run dbt deps - the only thing it needs the manifest for at all is an internally defined operation and internally defined macros (right?). So it shouldn't have to look at any non-global dependencies.
But maybe that could be because it's such a simple project, I don't know.
There was a problem hiding this comment.
Yeah, it's just a side effect of it being such a simple project. There are cases where there are actually models in the dependency project that end up in the manifest, and the corresponding tables in the database should end up in the catalog as well. But I think this code is fine as written. It's perfectly fine to assert that the user has to run dbt deps before one or both of these commands will work even if it's not theoretically necessary to generate the catalog right now.
|
@beckjake this looks good to me. |
This PR is a superset of #768 and can replace it.
This generates a catalog through the new comand
dbt catalog generateto create a file named "catalog.json" in the target directory, which has the column and table attributes discussed in #763. It uses the operation type internally instead of raw SQL embedded in the adapter'sget_catalogmethod, and adds arun_operationmethod to adapters that accepts the manifest as an argument.This feature is only implemented for PostgreSQL adapters.