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

Feature: reports #2752

Merged
merged 7 commits into from
Sep 16, 2020
Merged

Feature: reports #2752

merged 7 commits into from
Sep 16, 2020

Conversation

beckjake
Copy link
Contributor

resolves #2730

Description

Add report nodes to dbt.

They participate in the graph (as a "sink": not ref-able, but can use ref/source). That means they also honor selector behavior, which means things like dbt ls --select +my_report behave, dbt run --models '+state:modified' runs all ancestors of a modified report, etc.

I made two changes unrelated to this PR:

  • bumped the dbt version to 0.18.1a1, as it was getting a bit confusing to run locally/switch branches around
  • I moved the RPC tests to a folder starting with 100 - this is a quality of life change to make it easier to run all non-RPC integration tests locally. I removed some unused folders there, too.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

Jacob Beck added 4 commits September 11, 2020 14:21
remove unused test folders
move rpc tests from 048 to 100 for convenience
 - Migrating these to the test/rpc model is going to take work. In the
   interim, developers can now use `tests/integeration/0*` to run all non-rpc
   tests.
Add ParsedReport/UnparsedReport
add report parser and report node logic to manifest/results/dbt ls/selectors
NonSourceNode -> ManifestNode
add GraphMemberNode type that includes reports
@cla-bot cla-bot bot added the cla:yes label Sep 14, 2020
@beckjake beckjake marked this pull request as ready for review September 14, 2020 20:38
@beckjake
Copy link
Contributor Author

I'm not sure why it's not showing up here, but this is the azure pipelines build for e96f4a5

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.

This looks great! I messed around with some reports in our internal analytics project, there's a draft PR open there if you're curious.

Neither of these is a blocker for this PR, just things to discuss/modify between now and 0.18.0-rc1:

node selection syntax

Because reports are like inverse sources, I had been thinking about a report: node selector. This would get us a few small things:

  • Explicitness. A report can have the same name as a model (which feels ok), so we'd disambiguate between dbt run -m +report:customers and dbt run -m +customers.
  • List all resources upstream of all reports: dbt ls -s +report:*. By the same token, list all resources that are not exposed to any reports: dbt ls --exclude +report:*.

dbt-docs

Open issue: dbt-labs/dbt-docs#135

We just need to fix before we can release. They still work fine for projects that don't have reports defined.

@beckjake
Copy link
Contributor Author

beckjake commented Sep 15, 2020

@jtcohen6 does that mean that reports shouldn't act like nodes in FQN selection?

For example, sources can be selected with source: but they don't show up in the fqn/default selector. I think that would be good for reports, it'd remove some ambiguity that currently exists where a report with the same name as a model.

@jtcohen6
Copy link
Contributor

@beckjake That feels right to me. dbt run -m +ambiguous_name should default to the model, and excluding the same-named report has no functional difference since it isn't run. Is that a big change?

@beckjake
Copy link
Contributor Author

I think it just involves tweaking the FQN selector to not look at reports and adding a selector that's a lot like source:, but for reports.

@beckjake
Copy link
Contributor Author

That branch I just pushed is not quite right! Still need to handle package.report_name format, and need to fix report/source to output the correct selectors. Currently the dbt ls selector output is sort of wrong!

…eports from fqn slector

Make some corresponding fqn adjustments
Add dbt ls report output
Fix dbt ls source output
The default selector now also returns reports
Update tests
@beckjake beckjake merged commit f2caf2f into dev/0.18.1 Sep 16, 2020
@beckjake beckjake deleted the feature/reports branch September 16, 2020 16:41
@beckjake beckjake mentioned this pull request Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants