Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New command: dbt show #7208
New command: dbt show #7208
Changes from 25 commits
e6dcca4
f0ad043
eab733f
d726061
bc09cd7
f28af9c
2bcb790
e0db983
45a7f1a
3133d22
09bec45
e845b37
ea6b2b0
f25b44b
2ff263d
b3cc37d
9a6baa9
3f36494
f885193
aac83b5
ac008e0
5c96420
ffc66bd
e20af84
0bec7c4
310ddf3
05fcd34
b576018
c5d4f11
b1f29c1
06fd124
4309fac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we recreating as a new event type (with a new code)? The additional fields aren't a breaking change, right?
I think this will need to be updated to account for the changes we made last week to our event/proto system (#7190)I was mistaken about what had changed in that PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not, I just ran out of space in the numbering and wanted to move them to the right spot before shipping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Not blocking] It makes sense to me that we'd want to support similar arguments for both
compile
andshow
, including--output
. The current JSON output does feel a bit inconsistent between them:I don't have a very strong feeling about what to show here. The most important use case for JSON-formatted output is programmatic consumers of the
show
result set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
This is a regression, fixed with a new test.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtcohen6 did we agree on firing one event for each selected node is a good idea? what if we accidentally selected 1000 node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChenyuLInx The node name explicitly needs to be in the selector, so this situation is pretty unlikely. e.g., selector
my_model+
will be filtered down to justmy_model
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had my druthers, I'd prefer we do this in a way that kept more of this logic within dbt's selection syntax, e.g. "we only want to support the
FQN
selector method."As it is, we're going to apply the selection syntax, and then filter it down to only preview the nodes whose names explicitly appear in the
--select
arg. This means theshow
command won't support yaml selectors,tag:
-based selection (even if only one node has that tag), etc. We also won't be showing a log message explaining why, if a node is selected, it's not being previewed.In terms of the "happy path" for the
show
command, the intent is to onlyshow
resource(s) explicitly specified in the--select
syntax. Could I ask that we at least fire an event here, if a set of nodes is returned from the selection syntax, and then filtered out because the node's name wasn't explicitly passed to--select
?If we can do that, then this is fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment addressed? @aranke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChenyuLInx Yes, done now.