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.

Sign up for GitHub

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

Jump to bottom

Add a path: selector to the node selector (#454) #2258

Merged
merged 2 commits into from
Apr 3, 2020
Merged

Add a path: selector to the node selector (#454) #2258

merged 2 commits into from
Apr 3, 2020

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Mar 27, 2020 •
edited
Loading

resolves #454

Description

  • Instead of always being FQN by default, path-like selectors default to PATH
    • 'path-like' means it has an OS path separator in it (\ or / on windows, / on macos/linux)
  • Paths referring to non-root packages are ignored
  • Added a test to the dependency tests that checks both of these
$ dbt run --models models/x.sql
Running with dbt=0.17.0-a1
Found 3 models, 7 tests, 1 snapshot, 0 analyses, 134 macros, 0 operations, 4 seed files, 1 source

12:52:51 | Concurrency: 4 threads (target='postgres')
12:52:51 |
12:52:51 | 1 of 1 START table model dbt_postgres.x........................................ [RUN]
12:52:51 | 1 of 1 OK created table model dbt_postgres.x................................... [SELECT 1 in 0.09s]
12:52:51 |
12:52:51 | Finished running 1 table model in 0.36s.

Completed successfully

Done. PASS=1 WARN=0 ERROR=0 SKIP=0 TOTAL=1

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.

@cla-bot cla-bot bot added the cla:yes label Mar 27, 2020
@beckjake
Copy link
Contributor Author

This was previously marked as blocked, but I figured I'd just try to get this in before anyone starts working on #2167 in a breaking way.

@beckjake beckjake requested a review from drewbanin March 27, 2020 19:29
@drewbanin
Copy link
Contributor

wicked

drewbanin
drewbanin reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

One request here, but otherwise this is looking really good!

core/dbt/graph/selector.py Outdated
search = chain(self.parsed_nodes(included_nodes),
self.source_nodes(included_nodes))
for node, real_node in search:
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about doing a prefix match instead of an exact filepath match? That would let us do something like

dbt run -m models/path/to/models

In this scenario, dbt would select all of the nodes located at or below that specified filepath. You buy it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I don't have strong feelings about that. I guess it would give us parity with the fqn selector, which seems good.

@beckjake beckjake force-pushed the feature/model-select-paths branch from 07fcd09 to 9823cc6 Compare April 1, 2020 16:13
Jacob Beck added 2 commits April 1, 2020 10:14
Add a path: selector to the node selector
6c7d710 
Instead of always being FQN by default, path-like selectors default to PATH
Paths referring to non-root packages are ignored
Added a test to the dependency tests that checks both of these
PR feedback
f8ee056 
Also, added more tests
@beckjake beckjake force-pushed the feature/model-select-paths branch from 9823cc6 to f8ee056 Compare April 1, 2020 16:14
@beckjake beckjake requested a review from drewbanin April 1, 2020 16:29
@drewbanin drewbanin mentioned this pull request Apr 2, 2020
Open
drewbanin
drewbanin approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

This LGTM!

We should also update the autocomplete script to support path-based selectors. Just made an issue to track that change here: dbt-labs/dbt-completion.bash#5

@beckjake beckjake merged commit 6a26a5f into dev/octavius-catto Apr 3, 2020
@beckjake beckjake deleted the feature/model-select-paths branch April 3, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drewbanin drewbanin drewbanin approved these changes

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pass filenames to --models
2 participants
@beckjake @drewbanin